Page MenuHomePhabricator

Merge #13022: [qa] Attach node index to test_node AssertionError and print messages
ClosedPublic

Authored by jasonbcox on Aug 20 2019, 01:03.

Details

Summary

80a5e59 [qa] Attach node index to test_node AssertionError and print messages (James O'Beirne)

Pull request description:

In the midst of fighting with https://github.com/bitcoin/bitcoin/pull/12873 it became apparent that there're a number of assertions and print statements which are emitted by test nodes but don't identify the node in question. This change makes debugging a bit easier by adding identifying information to non-logger test_node-related error messages.

Tree-SHA512: 7cc86f2c81f4b3fdba15ec9a2d21a84c4b083629e845e82288087c3affbbdc5c68e74067621856cc97fe84fbc8cb4f5ca4977a51ef381e5d74515df8eb001239

Backport of Core PR13022
https://github.com/bitcoin/bitcoin/pull/13022/files

Depends on D3914

Test Plan

test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jasonbcox created this revision.Aug 20 2019, 01:03
jasonbcox planned changes to this revision.Aug 20 2019, 01:03
jasonbcox edited the summary of this revision. (Show Details)
jasonbcox requested review of this revision.Aug 20 2019, 01:32
Fabien requested changes to this revision.Aug 20 2019, 07:19
Fabien added a subscriber: Fabien.
Fabien added inline comments.
test/functional/test_framework/test_node.py
109 ↗(On Diff #10877)

Note that this notation discards support for python 3.4 (this syntax is from 3.5 https://docs.python.org/3/library/typing.html).

While not being a big issue (python 3.5 is default in Xenial and stretch, and is already pretty old), we might want to bump the minimum python version along with this diff (see https://github.com/bitcoin/bitcoin/pull/14954 and https://github.com/bitcoin/bitcoin/pull/15601).

Looks like core did it out of order ?

216 ↗(On Diff #10877)

That should be in par with the output from __getattr__, whether keep the error messages separated or just squash them into one, up to you.

This revision now requires changes to proceed.Aug 20 2019, 07:19
jasonbcox updated this revision to Diff 11210.Sep 11 2019, 17:39

Fixed get_wallet_rpc to be on par with getattr

jasonbcox added inline comments.Sep 11 2019, 17:40
test/functional/test_framework/test_node.py
109 ↗(On Diff #10877)

Good note. We should be safe to leave this as-is since everyone is using python 3.5+

deadalnix accepted this revision.Sep 12 2019, 08:27
Fabien accepted this revision.Sep 16 2019, 08:14
This revision is now accepted and ready to land.Sep 16 2019, 08:14