Page MenuHomePhabricator

Implement expected stderr output checks and out-of-tree single test execution
ClosedPublic

Authored by deadalnix on Jun 19 2017, 18:40.

Details

Summary

The abc-cmdline.py test illustrates how tests can check their expected stderr traces.

Refer also to the OutputChecker class in qa/rpc-tests/test_framework/outputchecker.py.

Test Plan
make check
qa/pull-tester/rpc-tests.py abc-cmdline.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJun 19 2017, 18:40
qa/rpc-tests/test_framework/outputchecker.py
88 ↗(On Diff #564)

This is a bug - should be using the find() method .

Fix nits spotted during self-check of Diff

deadalnix requested changes to this revision.Jun 19 2017, 20:03

Overall, this is a good approach. I think the whole design would benefit from passing the checker down directly instead of a boolean. That shouldn't be hard to change.

qa/rpc-tests/test_framework/test_framework.py
67 ↗(On Diff #568)

I think you should pass the checker down directly from the test instead of a boolean.

qa/rpc-tests/test_framework/util.py
234 ↗(On Diff #568)

Same thing, pass the output checker down. That'll allow test to pass whatever is needed for their checks. Passing boolean downs is usually a code smell.

See: http://www.informit.com/articles/article.aspx?p=1392524

This revision now requires changes to proceed.Jun 19 2017, 20:03

Will look at passing through OutputChecker object via initialize_chain().

qa/rpc-tests/test_framework/test_framework.py
67 ↗(On Diff #568)

This is passing down a list of checkers (or None if checking was not requested).

No booleans getting passed here.

qa/rpc-tests/test_framework/util.py
234 ↗(On Diff #568)

I suppose we could give that a try here, since the user specifies how many nodes he wants, so he can create a corresponding list of OutputChecker() objects to pass through.

Will test whether that works ok, and update if so.

If it poses some unforeseen problem, I recommend not to get stuck in these weeds at this time, after all there is plenty to do and no test currently uses such a feature (since they all pass with current changes).

qa/rpc-tests/test_framework/outputchecker.py
83 ↗(On Diff #568)

reminder to factor this out of the if-else

Your test plan formats badly. Put two spaces before commands and generally use markdown.

qa/rpc-tests/test_framework/test_framework.py
67 ↗(On Diff #568)

You are instantiating the check based on a bool. Doesn't matter if the bool is a member of an object you pass down or a direct argument, you are passing down a bool and instantiating based on it.

freetrader edited edge metadata.

Implement passing of OutputCheckers to initialize_chain() as requested by deadalnix

qa/rpc-tests/test_framework/test_framework.py
67 ↗(On Diff #568)

There is no other way to do this except set the bool attribute in the BitcoinTestFramework object up with a default value, unless you feel like modifying every test that uses it.

qa/rpc-tests/test_framework/test_framework.py
67 ↗(On Diff #568)

My think goes more among the line that there is no need to do this at all. abc-cmdline start nodes by itself and pass the checker down by itself already.

deadalnix requested changes to this revision.Jun 20 2017, 12:52
deadalnix added inline comments.
qa/rpc-tests/abc-cmdline.py
30 ↗(On Diff #584)

Remove.

48 ↗(On Diff #584)

Pass checker down explicitly.

62 ↗(On Diff #584)

You have the output checker, why do you assign it to some global state and then query that state indirectly ? please don't.

qa/rpc-tests/test_framework/test_framework.py
43 ↗(On Diff #584)

Remove.

67 ↗(On Diff #568)

Confirmed. This is not needed. Remove.

qa/rpc-tests/test_framework/util.py
183 ↗(On Diff #584)

Remove.

366 ↗(On Diff #584)

This should be part of the OutputChecker object.

397 ↗(On Diff #584)

This isn't really part of that diff. This belongs to D184 .

410 ↗(On Diff #584)

dito D184

427 ↗(On Diff #584)

Remove.

803 ↗(On Diff #584)

dito D184

234 ↗(On Diff #568)

It doesn't looks like this is necessary either.

This revision now requires changes to proceed.Jun 20 2017, 12:52

General comment:

This is turning into a back-and-forth over implementation preferences.
The Diff presented here is a working solution which I propose can be improved on incrementally.
As the non-acceptance of this PR is preventing regular execution of the abc-cmdline.py test,
I really would urge to follow and incremental improvement strategy in the test framework.

If anyone wishes to take this Diff and create an improved or minimalized version in the interim,
I would gladly accept any other such working solution that meets the same goals.

qa/rpc-tests/test_framework/util.py
183 ↗(On Diff #584)

See general comment.

397 ↗(On Diff #584)

D184 was abandoned as explained in its history.

I could have made it into a separate diff, but since certain ways of running tests (out of tree) do not work well without this, I decided to present it as an integrated solution.

If you prefer to split it out again, please do so.

410 ↗(On Diff #584)

as commented above

427 ↗(On Diff #584)

See general comment.

234 ↗(On Diff #568)

pass the output checker down

You literally requested it in previous comment.

It doesn't looks like this is necessary either.

Ok, up to you to decide what you want to do with it.

deadalnix edited reviewers, added: freetrader; removed: deadalnix.
deadalnix edited edge metadata.

Remove unecessary code
Remove global state

deadalnix edited the test plan for this revision. (Show Details)
deadalnix edited the summary of this revision. (Show Details)
deadalnix edited the test plan for this revision. (Show Details)

nits

Tested, thanks for the fixes, extraction and cleanup, deadalnix.

This revision is now accepted and ready to land.Jun 20 2017, 21:24
This revision was automatically updated to reflect the committed changes.