[qa] Fix error introduced into p2p-segwit.py, and prevent future similar errors #11319

pull sdaftuar wants to merge 2 commits into bitcoin:master from sdaftuar:2017-09-fix-p2p-segwit changing 2 files +9 −9

sdaftuar commented at 1:33 PM on September 13, 2017: member

#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.
qa: Treat mininode p2p exceptions as fatal a7820422e0

qa: Fix bug introduced in p2p-segwit.py

Changing __init__() -> set_test_params() in the tests should not have
applied to NodeConnCB-derived objects.

f97ab35fa9

fanquake added the label Tests on Sep 13, 2017
promag commented at 1:46 PM on September 13, 2017: member

utACK f97ab35.
promag commented at 1:49 PM on September 13, 2017: member

Nit, commit a782042 could be on it's own PR.
in test/functional/test_framework/mininode.py:1777 in f97ab35fa9
```
1770 | @@ -1773,8 +1771,10 @@ def got_data(self):
1771 |                      self.got_message(t)
1772 |                  else:
1773 |                      logger.warning("Received unknown command from %s:%d: '%s' %s" % (self.dstaddr, self.dstport, command, repr(msg)))
1774 | +                    raise ValueError("Unknown command: '%s'" % (command))
1775 |          except Exception as e:
1776 |              logger.exception('got_data:', repr(e))
1777 | +            raise
```
jnewbery commented at 2:58 PM on September 13, 2017:

This try/except was added by @MarcoFalke in https://github.com/bitcoin/bitcoin/commit/faaa3c9b6546d9a64cece4ff0223f0b167feb6ff, but there are no commit or PR notes explaining the change. Marco - why are we currently catching these errors instead of raising them?

MarcoFalke commented at 5:58 PM on September 29, 2017:

@jnewbery As of faaa3c9b6546d9a64cece4ff0223f0b167feb6ff there was a try,except,pass in handle_read that ate all exceptions without notice. By adding the error print I didn't change behavior but added useful debug information.

MarcoFalke commented at 6:26 PM on September 29, 2017:

Though, obviously they should be thrown. Going to merge this.
jnewbery commented at 5:36 PM on September 13, 2017: member

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 6c1d1ba6fccd76e3690bac8341687c0921758e30, so as long as the tests all pass it should be fine to make these changes.
jnewbery commented at 5:40 PM on September 13, 2017: member

Tested ACK f97ab35fa9687fd5c110ad6cca5be5b4a5c2142d. Extended test suite passes locally.
sdaftuar commented at 3:38 PM on September 29, 2017: member

@marcofalke Anything else needed here?
MarcoFalke commented at 6:23 PM on September 29, 2017: member

@sdaftuar No. All looks fine. Thanks for the ping.

utACK f97ab35fa9687fd5c110ad6cca5be5b4a5c2142d
MarcoFalke merged this on Sep 29, 2017
MarcoFalke closed this on Sep 29, 2017
MarcoFalke referenced this in commit ff4cd6075b on Sep 29, 2017
MarcoFalke referenced this in commit b5cc2ae7db on Oct 3, 2017
MarcoFalke referenced this in commit 5325759fc8 on Oct 3, 2017
MarcoFalke referenced this in commit f694ada75d on Oct 3, 2017
MarcoFalke referenced this in commit bd7d53379b on Oct 3, 2017
MarcoFalke referenced this in commit c244c932e3 on Oct 3, 2017
MarcoFalke referenced this in commit e1177b7e3a on Oct 3, 2017
MarcoFalke referenced this in commit 30cd6718cf on Oct 3, 2017
MarcoFalke referenced this in commit f5783f0906 on Oct 3, 2017
MarcoFalke referenced this in commit 2f0b30a58a on Oct 3, 2017
MarcoFalke referenced this in commit 8d2e51d862 on Oct 3, 2017
attilaaf referenced this in commit 90b86ad1dd on May 25, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#11121 TestNode tidyups