Page MenuHomePhabricator

Add an argument to the test framework to enable great wall activation
Needs ReviewPublic

Authored by Fabien on Fri, Feb 8, 10:50.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Project
Summary

This will set "-greatwallactivationtime" in the past (01/01/2019).
Together with D2460, it will allow for running the functional tests both
pre and post update.

This is a replacement for D2459.
Depends on D2535

I considered using initialize_datadir() to set the argument into the
config file (like proposed in D2431) but:

  • it may not work with tests that override the setup_chain() method

(currently there is only rpc_users.py, which would work anyway because
it calls super.setup_chain())

  • it may not work with tests that override the datadir or config file

location. This is the case of feature_conf_args.py.

If a test sets the -greatwallactivationtime argument in the node
extra_args, or by passing them to the start_node(), it will still
override this argument due to the precedence rules.

Test Plan

Modify the test_node.py start() method. Before the
subprocess.Popen() call, add the following line:
assert("-greatwallactivationtime=1546300800" in self.default_args)
Run ./test/functional/test_runner.py <test> on any test and check it
fails due to the newly added assertion.
Run ./test/functional/test_runner.py --extended --with-greatwallactivation and check there is no error.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
test_runner_activate_greatwall
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4951
Build 7965: Bitcoin ABC Teamcity Staging
Build 7964: arc lint + arc unit

Event Timeline

Fabien created this revision.Fri, Feb 8, 10:50
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Feb 8, 10:50
Herald added a subscriber: schancel. · View Herald Transcript
Fabien edited the test plan for this revision. (Show Details)Fri, Feb 8, 11:08
deadalnix requested changes to this revision.Fri, Feb 8, 14:37
deadalnix added inline comments.
test/functional/test_framework/test_framework.py
254 ↗(On Diff #7239)

No need for parenthesis.

256 ↗(On Diff #7239)

This seems like it would override the value for any test that want to set it explicitly.

435 ↗(On Diff #7239)

dito

437 ↗(On Diff #7239)

I'm not sure we want to cache command line arguments. Or if so, then there needs to be some cache invalidation logic. Thinking about this more, it seems like this is the proper approach: add a facility to add elements to the config file used by the node.

This revision now requires changes to proceed.Fri, Feb 8, 14:37
Fabien edited the summary of this revision. (Show Details)Fri, Feb 8, 14:55
Fabien added inline comments.Fri, Feb 8, 15:11
test/functional/test_framework/test_framework.py
256 ↗(On Diff #7239)

The node has 2 attributes, args and extra_args. The tests can set the latter one.
When the process is started, the node arguments are set to args + extra_args, so the test can still override.

437 ↗(On Diff #7239)

The arguments caching thing is already built in TestNode. There is no facility to access it, so the args attribute is accessed directly when needed (here and in feature_conf_args.py to remove the -datadir argument). This can be easity refactored if needed.
Accessing the configuration file would be a more elegant solution, but it has some drawbacks (see summary); especially this would let the feature_conf_args test not running with the flag.

Fabien updated this revision to Diff 7244.Fri, Feb 8, 16:45

Remove parenthesis

jasonbcox requested changes to this revision.Fri, Feb 8, 21:13
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
test/functional/test_framework/test_framework.py
256 ↗(On Diff #7239)

I think Amaury is pointing out that by altering the args after creating the node, setting -greatwallactivationtime overrides values that are set explicitly in tests. This is probably not the intent, as the test should pass in the state it was set at.

However, I see a problem with this approach no matter how you slice it:

  1. A non-overriding implementation will only catch tests that will fail on flagday, but not tests that need to be updated to cover new behavior.
  2. An overriding implementation like this one will break tests that are intended only to operated under the activation time explicitly specified in the tests.

Given the above, I think #1 is the right way to go, as tests that need to be updated can be easily grep'd for. Our main concern is what breaks on flagday.

This revision now requires changes to proceed.Fri, Feb 8, 21:13
jasonbcox added inline comments.Fri, Feb 8, 21:18
test/functional/test_framework/test_framework.py
256 ↗(On Diff #7239)

Ah I see. Taking a look at the implementation of TestNode, #1 is precisely what you have done. self.args is set as part of TestNode's init and the self.args + self.extra_args do not get appended until the node has started with a call to start().

In that case, this implementation is relying on behavior internal to TestNode. There are two much clearer ways to deal with this:

  1. Prepend this override to the front of extra_args, or
  2. Implement a self.override_args in TestNode that gets appended internally as: self.args + self.override_args + self.extra_args
Fabien planned changes to this revision.Fri, Feb 8, 22:42
Fabien added inline comments.
test/functional/test_framework/test_framework.py
256 ↗(On Diff #7239)
  1. Prepending self.extra_args is likely to fail, because the self.extra_args that are set at init can be overridden at each node start() or restart(), giving many opportunities for the test to clear the flag (override in these cases is achieved by assignation, not appending)
  2. The actual TestNode implementation is weird regarding arguments. I don't think that adding a new attribute such as the suggested self.override_args is the best solution, it is another layer of arguments which is confusing.

So far the best approach imo is to refactor the TestNode to add accessors for self.args, maybe renaming it to clarify its intent. This would make the changes no longer rely on TestNode internal behavior.
Any thought ?

Fabien updated this revision to Diff 7288.Sun, Feb 10, 11:31

Rebase on top of D2535

Fabien edited the summary of this revision. (Show Details)Sun, Feb 10, 11:32
Fabien edited the test plan for this revision. (Show Details)
jasonbcox accepted this revision.Tue, Feb 12, 19:15