fullblocktests: Decouple from blockchain. #2951
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoeGruffins
approved these changes
May 20, 2022
c1e3a66 to
a98bbba
Compare
a98bbba to
ca29669
Compare
dajohi
approved these changes
May 27, 2022
matheusd
approved these changes
May 30, 2022
This renames the error returned when there is no treasury payout present in a coinbase and/or treasurybase to match its actual meaning.
The blockchain/fullblocktests package currently has a cyclic dependency on blockchain since the tests directly return the error codes in blockchain that are expected to be violated while blockchain itself needs to import the package in order to run the tests. This resolves that cyclic dependency by defining all of the errors the fullblocktests produce in the package itself and then converting them to the associated error in blockchain when running the tests and checking for the expected error. It also moves the code that runs the fullblocktests into the blockchain itself instead of a separate blockchain_test to match all other tests now that there is no longer a cyclic dependency that forced it to be be separated. While duplication of the errors in question is a little less convenient, this approach ensures the blockchain package can be made internal in the future while still providing the publicly-available fullblocktests package for use both in testing the internal blockchain implementation as well as integration tests with other implementations.
ca29669 to
b767607
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
blockchain/fullblocktestspackage currently has a cyclic dependency onblockchainsince the tests directly return the error codes inblockchainthat are expected to be violated whileblockchainitself needs to import the package in order to run the tests.This resolves that cyclic dependency by defining all of the errors the
fullblocktestsproduce in the package itself and then converting them to the associated error in blockchain when running the tests and checking for the expected error.It also moves the code that runs the
fullblocktestsinto theblockchainpackage itself instead of a separateblockchain_testpackage to match all other tests now that there is no longer a cyclic dependency that forced it to be be separated.While duplication of the errors in question is a little less convenient, this approach ensures the
blockchainpackage can be made internal in the future while still providing the publicly-availablefullblocktestspackage for use both in testing the internalblockchainimplementation as well as integration tests with other implementations.Finally, this also has a few other commits with some minor cleanup.