[qa] Fix error introduced into p2p-segwit.py, and prevent future similar errors #11319
Conversation
Changing __init__() -> set_test_params() in the tests should not have applied to NodeConnCB-derived objects.
|
utACK f97ab35. |
|
Nit, commit a782042 could be on it's own PR. |
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 |
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?
There was a problem hiding this comment.
Though, obviously they should be thrown. Going to merge this.
|
Tested ACK f97ab35. Extended test suite passes locally. |
|
@MarcoFalke Anything else needed here? |
…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
Github-Pull: bitcoin#11319 Rebased-From: a782042
Changing __init__() -> set_test_params() in the tests should not have applied to NodeConnCB-derived objects. Github-Pull: bitcoin#11319 Rebased-From: f97ab35
5 participants
#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.