Page MenuHomePhabricator

Add a cli option to the test framework to add an argument to all nodes
AbandonedPublic

Authored by Fabien on Thu, Jan 31, 14:39.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

This allow to initially apply the same arg to all nodes in all tests.
e.g.: test_runner.py --nodes_arg="-greatwallactivationtime=1546300800"
will initially set all the nodes with an extra argument
-greatwallactivationtime=1546300800.
This option can be set multiple times to add multiple arguments.
These arguments are appended to the fixed argument list from TestNode,
then the extra_args from the tests are appended to these.
This still allow to override any argument from a test as the last cli argument
takes precedence over the previous occurrences.

Test Plan

Should pass:

./test/functional/test_runner.py --nodes_arg="-greatwallactivationtime=1546300800"

Should fail:

./test/functional/test_runner.py --nodes_arg="-uacomment=foo" feature_uacomment

Should fail:

./test/functional/test_runner.py --nodes_arg="-rpcuser=foo" rpc_users

Should pass:

./test/functional/test_runner.py --nodes_arg="-rpcuser=foo" --nodes_arg="-rpcuser=rpcuser💻" rpc_users

Note: the "computer" character in the above command is not a mistake

Diff Detail

Repository
rABC Bitcoin ABC
Branch
test_runner_node_args
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4856
Build 7775: Bitcoin ABC Teamcity Staging
Build 7774: arc lint + arc unit

Event Timeline

Fabien created this revision.Thu, Jan 31, 14:39
Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, Jan 31, 14:39
Herald added a subscriber: schancel. · View Herald Transcript
jasonbcox requested changes to this revision.Sun, Feb 3, 21:19
jasonbcox added a subscriber: jasonbcox.

There also appears to be a copy-paste error in the test plan.

test/functional/test_framework/test_framework.py
231 ↗(On Diff #7060)

This needs to be fixed to append self.extra_args to extra_args, or else overrides of setup_noodes will erase nodes_args that were passed in.

511 ↗(On Diff #7060)

ditto

This revision now requires changes to proceed.Sun, Feb 3, 21:19
Fabien edited the test plan for this revision. (Show Details)Sun, Feb 3, 21:40
Fabien edited the test plan for this revision. (Show Details)Sun, Feb 3, 22:15
Fabien edited the test plan for this revision. (Show Details)Mon, Feb 4, 09:26
Fabien updated this revision to Diff 7149.Mon, Feb 4, 09:30

Do not override arguments when using TestNode.extra_args, but extend them instead

Fabien edited the summary of this revision. (Show Details)Mon, Feb 4, 09:32
deadalnix requested changes to this revision.Mon, Feb 4, 12:33

I really fail to see what's the goal here.

  • This will almost certainly break everything for most flags - without it actually being an error, just the flag undermining the test assumptions. There fore it doesn't seems useful as a general purpose mechanism.
  • This will not work predictably for tests that override the setup_network or setup_nodes method.
  • This will not work properly for multiarg.

I believe the approach to be fundamentally broken. I cannot suggest alternative as there is no motivation statement.

This revision now requires changes to proceed.Mon, Feb 4, 12:33
Fabien requested review of this revision.Mon, Feb 4, 13:23

@deadalnix The idea is to provide a mechanism for running the tests with greatwallactivationtime enabled. This diff together with D2460 allow for running the tests both pre and post update using the CI.
This is a followup of the discussion on slack regarding D2431.

Regarding your points:

  1. That's correct, it's useless for other flags. I did it this way rather than a specific option for -greatwallactivationtime so the feature can be used the same for the future updates with the later flags.
  2. Tests that override these methods do it on purpose, I found it safer to not enforce arguments for these. It effectively makes the argument ineffective on these tests ; this behavior can be changed if required.
  3. Yes multiargs are prepended with this argument, which is not very intuitive

Here are the motivation and some explanations. If you think the idea is bad, I can abandon these diffs or update them to a better solution.

deadalnix requested changes to this revision.Tue, Feb 5, 00:24
  1. Show the mechanism is not adapted. If 99% of the possible of a tool are shooting yourself in the foot, then that means the tool is badly design, end of story.
  2. If the intended use case is to be able to check the state of the test suite post activation, then it fail flat on its face as random tests will not do it, silently.

So this is completely wrong. I don't think this is salvageable as the whole approach is wrong. You need to go back to the drawing board with your use case in mind.

This revision now requires changes to proceed.Tue, Feb 5, 00:24
Fabien abandoned this revision.Tue, Feb 5, 08:40

Need a better solution