[qa] Fix error introduced into p2p-segwit.py, and prevent future similar errors#11319
Merged
maflcko merged 2 commits intobitcoin:masterfrom Sep 29, 2017
Merged
[qa] Fix error introduced into p2p-segwit.py, and prevent future similar errors#11319maflcko merged 2 commits intobitcoin:masterfrom
maflcko merged 2 commits intobitcoin:masterfrom
Conversation
Changing __init__() -> set_test_params() in the tests should not have applied to NodeConnCB-derived objects.
Contributor
|
utACK f97ab35. |
Contributor
|
Nit, commit a782042 could be on it's own PR. |
jnewbery
reviewed
Sep 13, 2017
Contributor
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Testing this now. Definitely ACK to the p2p-segwit.py change. That's a crass bug introduced by me.
One question for @MarcoFalke inline. The other two exception catches have been in mininode.py since it was introduced in 6c1d1ba, so as long as the tests all pass it should be fine to make these changes.
| raise ValueError("Unknown command: '%s'" % (command)) | ||
| except Exception as e: | ||
| logger.exception('got_data:', repr(e)) | ||
| raise |
Contributor
There was a problem hiding this comment.
This try/except was added by @MarcoFalke in faaa3c9, but there are no commit or PR notes explaining the change. Marco - why are we currently catching these errors instead of raising them?
Member
Member
There was a problem hiding this comment.
Though, obviously they should be thrown. Going to merge this.
Contributor
|
Tested ACK f97ab35. Extended test suite passes locally. |
Member
Author
|
@MarcoFalke Anything else needed here? |
Member
maflcko
pushed a commit
that referenced
this pull request
Sep 29, 2017
…nt future similar errors f97ab35 qa: Fix bug introduced in p2p-segwit.py (Suhas Daftuar) a782042 qa: Treat mininode p2p exceptions as fatal (Suhas Daftuar) Pull request description: #11121 inadvertently broke the constructor for the `TestNode()` object in `p2p-segwit.py`, silently breaking at least one of the tests. Although the python code was raising exceptions due to a `TestNode()` object not existing (or having the right type), mininode was masking these from anyone running the test through the test_runner (like travis), because it catches all exceptions during message delivery and just prints a log message and continues. Such "graceful" handling of errors is almost certainly something we don't want in our test suite, so the first commit here attempts to prevent that type of failure from ever being masked. The second commit fixes the particular bug in `p2p-segwit.py`. Tree-SHA512: b6646e3cb1e05c35c28e8899c44104bf2e2d0384643ca87042ab7f6ec0960d89f5bf25a7b95bab6e32d401c20a6018226160500f6ddceb923e81ffb04adb4f2f
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Oct 3, 2017
Github-Pull: bitcoin#11319 Rebased-From: a782042
maflcko
pushed a commit
to maflcko/bitcoin-core
that referenced
this pull request
Oct 3, 2017
Changing __init__() -> set_test_params() in the tests should not have applied to NodeConnCB-derived objects. Github-Pull: bitcoin#11319 Rebased-From: f97ab35
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
#11121 inadvertently broke the constructor for the
TestNode()object inp2p-segwit.py, silently breaking at least one of the tests.Although the python code was raising exceptions due to a
TestNode()object not existing (or having the right type), mininode was masking these from anyone running the test through the test_runner (like travis), because it catches all exceptions during message delivery and just prints a log message and continues. Such "graceful" handling of errors is almost certainly something we don't want in our test suite, so the first commit here attempts to prevent that type of failure from ever being masked.The second commit fixes the particular bug in
p2p-segwit.py.