Skip to content

consensus: remove panics from test helper functions#6969

Merged
tychoish merged 5 commits intowb/proposer-based-timestampsfrom
wb/panic-remove
Sep 22, 2021
Merged

consensus: remove panics from test helper functions#6969
tychoish merged 5 commits intowb/proposer-based-timestampsfrom
wb/panic-remove

Conversation

@williambanfield
Copy link
Contributor

@williambanfield williambanfield commented Sep 21, 2021

This change removes many of the panics in the internal/consensus/*_test.go methods. Instead, it plumbs through a *testing.T and makes use of t.Fatalf in the place of panic.

Using panic makes the test output more difficult to parse. Using t.Fatal and marking the ensure* and validate* functions as helpers using t.Helper() allows the testing to play a bit more nicely with tooling as well by indicating which line in the test the failure originated from.

@williambanfield williambanfield changed the base branch from master to wb/proposer-based-timestamps September 21, 2021 21:01
@williambanfield williambanfield added the S:automerge Automatically merge PR when requirements pass label Sep 21, 2021
@tychoish tychoish merged commit 87f4beb into wb/proposer-based-timestamps Sep 22, 2021
@tychoish tychoish deleted the wb/panic-remove branch September 22, 2021 12:56
@williambanfield williambanfield restored the wb/panic-remove branch September 9, 2022 16:13
sergio-mena added a commit to cometbft/cometbft that referenced this pull request Jan 23, 2024
…endermint/tendermint#8142 (#2018)

Closes #2003

This is a pure forward-port of tendermint/tendermint#7605, which was a
"squashed merge" of several other PRs.

There are several refactorings included in tendermint/tendermint#7605
that have been left out of the cherry-pick (not necessary and their
absence makes this PR smaller):
- tendermint/tendermint#6969
- Everything that has to do with context threading, and `RandState` vs.
`MakeState` (since the logic this changes is anyway not present on
`main` anyway)

Finally, two further commits had to be be forward-ported together with
tendermint/tendermint#7605:
- tendermint/spec#393 When PBTS was implemented (end of 2021 - beginning
of 2022) the protobufs were held in a different repo (spec). This PR on
that repo contains those changes, which are needed by
tendermint/tendermint#7605
- tendermint/tendermint#8142 Minor adjustments to Synchrony parameters
that was committed separately on `master`

---

#### Original description of tendermint/tendermint#7605:

This pull request merges in the changes for implementing Proposer-based
timestamps into `master`. The power was primarily being done in the
`wb/proposer-based-timestamps` branch, with changes being merged into
that branch during development. This pull request represents an
amalgamation of the changes made into that development branch. All of
the changes that were placed into that branch have been cleanly rebased
on top of the latest `master`. The changes compile and the tests pass
insofar as our tests in general pass.

These changes have been extensively reviewed during development. There
is not much new here. In the interest of making effective use of time, I
would recommend against trying to perform a complete audit of the
changes presented and instead examine for mistakes that may have
occurred during the process of rebasing the changes. I gave the complete
change set a first pass for any issues, but additional eyes would be
very appreciated.

In sum, this change set does the following:
closes #6942
merges in #6849

---

#### PR checklist

- [x] Tests written/updated
- [x] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments

---------

Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: Daniel <daniel.cason@informal.systems>
Co-authored-by: glnro <8335464+glnro@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S:automerge Automatically merge PR when requirements pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants