test: Adapt test framework for chains other than "regtest" #16509
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
maflcko
force-pushed
the
1908-testChainName
branch
from
fa84e2d
to
faba3f8
Compare
flake8 F841 lints, as of flake8 3.6.0
maflcko
force-pushed
the
1908-testChainName
branch
from
faba3f8
to
fa12708
Compare
Co-Authored-By: Jorge Timón <jtimon@jtimon.cc>
maflcko
force-pushed
the
1908-testChainName
branch
from
fa12708
to
fa8f755
Compare
|
Force pushed to make linters happy |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Oh, nice. utACK, but it needs rebase. |
It is not tagged with "Needs rebase". Am I missing something? |
|
Mhmm, that's weird, but never mind, github doesn't tell me there's conflicts now. |
kallewoof
approved these changes
Aug 2, 2019
maflcko
force-pushed
the
1908-testChainName
branch
from
fa8f755
to
faf3683
Compare
|
Changed last commit to address feedback by @kallewoof |
|
Concept ACK: nice work! |
maflcko
pushed a commit
that referenced
this pull request
Aug 5, 2019
faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains #8994 * signet #16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 10, 2019
… "regtest" faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains bitcoin#8994 * signet bitcoin#16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6
|
For this to be useful for #8994 it needed to be complete, which is not. And you have been opposing its completion very actively in #16681 using different excuses/reasons. For signet, no old test (ie feature_assumevalid.py, etc) needs to be updated, only the test framework. So can you re-explain your motivation for changing old tests? |
maflcko
pushed a commit
that referenced
this pull request
Feb 4, 2020
…nt tests 1abcecc Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón) Pull request description: Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest. Separated from #8994 . Continues #16509 . It is still not complete (ie to be complete, we need the -chain parameter in #16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in #8994 ] ). But while being incomplete like #16509 , it's quite simple to review and another step forward IMO. ACKs for top commit: Sjors: re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files. elichai: Code review ACK 1abcecc ryanofsky: Code review ACK 1abcecc. ryanofsky: Code review ACK 1abcecc Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
The changes here were required to even be able to set a different chain (e.g. testnet) for new tests. The changes in #16681, are refactoring old tests that don't need their chain-name changed. I think there have been two oppositions to rename regtest to regtest2. However, the changes in #16681 are (concept) ACKed by a few people and I am the only one who objected them in the past. I will go ahead and just merge them, if other people think they are useful to them. After all those changes alone won't cause any harm. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 9, 2020
…l current tests 1abcecc Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón) Pull request description: Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest. Separated from bitcoin#8994 . Continues bitcoin#16509 . It is still not complete (ie to be complete, we need the -chain parameter in bitcoin#16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in bitcoin#8994 ] ). But while being incomplete like bitcoin#16509 , it's quite simple to review and another step forward IMO. ACKs for top commit: Sjors: re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files. elichai: Code review ACK 1abcecc ryanofsky: Code review ACK 1abcecc. ryanofsky: Code review ACK 1abcecc Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
deadalnix
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
May 3, 2020
Summary: * test: Fix “local variable 'e' is assigned to but never used” flake8 F841 lints, as of flake8 3.6.0 * test: Adapt test framework for chains other than "regtest" Co-Authored-By: Jorge Timón <jtimon@jtimon.cc> * test: Avoid hardcoding the chain name in combine_logs This is a backport of Core [[bitcoin/bitcoin#16509 | PR16509]] The original PR contains a lot of formatting that was made obsolete by our process. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5942
sidhujag
pushed a commit
to syscoin-core/syscoin
that referenced
this pull request
Nov 10, 2020
…l current tests 1abcecc Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón) Pull request description: Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest. Separated from bitcoin#8994 . Continues bitcoin#16509 . It is still not complete (ie to be complete, we need the -chain parameter in bitcoin#16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in bitcoin#8994 ] ). But while being incomplete like bitcoin#16509 , it's quite simple to review and another step forward IMO. ACKs for top commit: Sjors: re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files. elichai: Code review ACK 1abcecc ryanofsky: Code review ACK 1abcecc. ryanofsky: Code review ACK 1abcecc Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 22, 2020
Summary: This is a backport of Core [[bitcoin/bitcoin#16681 | PR16681]] and [[bitcoin/bitcoin#18069 | PR18069]] I also replaced one occurence of 'regtest' in test_framework.py, that was missed in the backport of [[bitcoin/bitcoin#16509 | PR16509]] (D5942) Test Plan: `ninja && ninja check-functional-extended` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8738
KolbyML
pushed a commit
to KolbyML/bitcoin
that referenced
this pull request
Jan 17, 2021
… "regtest" faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains bitcoin#8994 * signet bitcoin#16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6
KolbyML
pushed a commit
to KolbyML/bitcoin
that referenced
this pull request
Jan 17, 2021
… "regtest" faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains bitcoin#8994 * signet bitcoin#16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6
UdjinM6
pushed a commit
to UdjinM6/dash
that referenced
this pull request
Jan 21, 2021
…t' in all current tests 1abcecc Tests: Use self.chain instead of 'regtest' in almost all current tests (Jorge Timón) Pull request description: Simply avoiding the hardcoded string in more places for consistency. It can also allow for more easily reusing tests for other chains other than regtest. Separated from bitcoin#8994 . Continues bitcoin#16509 . It is still not complete (ie to be complete, we need the -chain parameter in bitcoin#16680 and make whether acceptnonstdtxs is allowed for that chain or not customizable for regtest [or for custom chains like in bitcoin#8994 ] ). But while being incomplete like bitcoin#16509 , it's quite simple to review and another step forward IMO. ACKs for top commit: Sjors: re-ACK 1abcecc. I think it's an improvement even if incomplete and if some PR's might accidentally bring "regtest" back. Subsequent improvements hopefully don't have to touch 16 files. elichai: Code review ACK 1abcecc ryanofsky: Code review ACK 1abcecc. ryanofsky: Code review ACK 1abcecc Tree-SHA512: 5620de6dab235ca8bd8670d6366c7b9f04f0e3ca9c5e7f87765b38e16ed80c17d7d1630c0d5fd7c5526f070830d94dc74cc2096d8ede87dc7180ed20569509ee
PastaPastaPasta
pushed a commit
to dashpay/dash
that referenced
this pull request
Jan 22, 2021
* Merge bitcoin#16509: test: Adapt test framework for chains other than "regtest" faf3683 test: Avoid hardcoding the chain name in combine_logs (MarcoFalke) fa8a1d7 test: Adapt test framework for chains other than "regtest" (MarcoFalke) 68f5466 test: Fix “local variable 'e' is assigned to but never used” (Ben Woosley) Pull request description: This is required for various work in progress: * testchains bitcoin#8994 * signet bitcoin#16411 * some of my locally written tests While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes. ACKs for top commit: jonatack: ACK faf3683, ran tests and reviewed the code. Tree-SHA512: 35add66c12cab68f2fac8f7c7d47c604d3f24eae9336ff78f83e2c92b3dc08a25e7f4217199bac5393dd3fb72f945bba9c001d6fbb8efd298c88858075fcb3d6 * Add devnet support for tests * test: make sure devnet can connect to each other and start * Partial merge bitcoin#16681: Tests: Use self.chain instead of 'regtest' in almost all current tests, revert one TODO while at it Co-authored-by: MarcoFalke <falke.marco@gmail.com> Co-authored-by: Jorge Timón <jtimon@jtimon.cc>
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jan 22, 2021
This reverts commit e197f97 Signed-off-by: pasta <pasta@dashboost.org>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This is required for various work in progress:
While it will be unused in the master branch as of now, it will make all of those pull requests shorter. Thus review for non-regtest tests can focus on the actual changes and not some test framework changes.