diff options
| author | Andrew Chow <github@achow101.com> | 2023-10-13 12:27:12 -0400 |
|---|---|---|
| committer | Andrew Chow <github@achow101.com> | 2023-10-13 12:32:01 -0400 |
| commit | 78b7e955185ab92de4e1b8b866a46d3113a5fdf5 (patch) | |
| tree | 9547ae5696f45d74f056dcc96bd5ef8967a85fc5 /src/qt | |
| parent | 73dfa6da0801e3116e7e84cd5e089b98a9f70212 (diff) | |
| parent | ac4caf3366a85617641394a97aa9f029550d77d4 (diff) | |
Merge bitcoin/bitcoin#28645: test: fix `assert_debug_log` call-site bugs, add type checks
ac4caf3366a85617641394a97aa9f029550d77d4 test: fix `assert_debug_log` call-site bugs, add type checks (Sebastian Falbesoner)
Pull request description:
Two recently added tests (PR #28625 / commit 2e31250027ac580a7a72221fe2ff505b30836175 and PR #28634 / commit 3bb51c29df596aab2c1fde184667cee435597715) introduced bugs by wrongly using the `assert_debug_log` helper:
https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/feature_assumeutxo.py#L84-L85 (already fixed in https://github.com/bitcoin/bitcoin/pull/28639)
https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L148
https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L159
Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. Thanks to maflcko for discovering: https://github.com/bitcoin/bitcoin/pull/28625#discussion_r1356489861
In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in https://github.com/bitcoin/bitcoin/pull/28625#discussion_r1356864233. Using mypy might be an alternative, but I guess it takes quite a bit of effort to properly integrate this into CI for the whole functional test suite (including taking care of false-positives), so I decided to go with the simpler "manual asserts" hack. Suggestions are very welcome of course.
ACKs for top commit:
achow101:
ACK ac4caf3366a85617641394a97aa9f029550d77d4
maflcko:
lgtm ACK ac4caf3366a85617641394a97aa9f029550d77d4
dergoegge:
ACK ac4caf3366a85617641394a97aa9f029550d77d4
Tree-SHA512: a9677af76a0c370e71f0411339807b1dc6b2a81763db4ec049cd6d766404b916e2bdd002883db5a79c9c388d7d8ebfcbd5f31d43d50be868eeb928e3c906a746
Diffstat (limited to 'src/qt')
0 files changed, 0 insertions, 0 deletions