Add configuration file/argument testing #11883

meshcollider · 2017-12-12T23:59:17Z

Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests.

Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in #11829. It also tests that command line arguments override the ones in the config file.

I plan on working on a fix for #11819 / #1044 and then expanding this test with cases for that.

maflcko · 2017-12-13T00:48:54Z

test/functional/conf_args.py

+    def run_test(self):
+        self.stop_node(0)
+        # Remove the -datadir argument so it doesn't override the config file
+        self.nodes[0].args = [arg for arg in self.nodes[0].args if arg.startswith("-datadir")]


Did you mean if **not** arg.startswith("-datadir")?

Doh, yes, oops :)

maflcko · 2017-12-13T00:49:42Z

test/functional/conf_args.py

+        # Ensure command line argument overrides datadir in conf
+        os.mkdir(new_data_dir_2)
+        self.start_node(0, ['-datadir='+new_data_dir_2, '-wallet=w2'])
+        assert(os.path.isfile(os.path.join(new_data_dir_2, 'wallets', 'w2')))


femto-nit: missing space after assert, redundant round braces.

maflcko · 2017-12-13T19:19:16Z

test/functional/conf_args.py

+    def run_test(self):
+        self.stop_node(0)
+        # Remove the -datadir argument so it doesn't override the config file
+        self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-datadir")]


Note that the configuration is written to the bitcoin.conf in the datadir that you are removing from the args. I suspect this will cause issues.

Hint: git grep 'def initialize_datadir'

Yeah I've been trying to figure a way around this, if -datadir is specified on the command line it will override the conf file, but if it isn't then it can't find the conf file anyway. Still trying to think if this is possible or not. EDIT: Fixed

ryanofsky

utACK 7dc6a4253ea1c101b6b0591e4c9a84b9015d5891

ryanofsky · 2017-12-18T13:50:40Z

test/functional/conf_args.py

+        new_data_dir = os.path.join(default_data_dir, 'newdatadir')
+        new_data_dir_2 = os.path.join(default_data_dir, 'newdatadir2')
+
+        # Check that using -datadir argument on non-existant directory fails


spelling existent

ryanofsky · 2017-12-18T13:56:38Z

test/functional/conf_args.py

+        # Remove the -datadir argument so it doesn't override the config file
+        self.nodes[0].args = [arg for arg in self.nodes[0].args if not arg.startswith("-datadir")]
+
+        default_data_dir = os.path.join(self.options.tmpdir, 'node0')


Maybe call get_datadir_path(self.options.tmpdir, 0) to avoid hardcoding this.

meshcollider · 2017-12-19T23:26:34Z

Addressed @ryanofsky comments and squashed fixup commit, thanks :)

promag

utACK be9a13c.

promag · 2017-12-20T02:48:02Z

test/functional/conf_args.py

+        new_data_dir_2 = os.path.join(default_data_dir, 'newdatadir2')
+
+        # Check that using -datadir argument on non-existent directory fails
+        self.nodes[0].datadir = new_data_dir


promag · 2017-12-20T02:48:14Z

test/functional/conf_args.py

+
+        # Ensure command line argument overrides datadir in conf
+        os.mkdir(new_data_dir_2)
+        self.nodes[0].datadir = new_data_dir_2


Can't remove either of those, they are used by the test runner to connect to the RPC and monitor the output of debug.log and stuff I believe, otherwise it can't find it. It's set separately from any command line arguments to bitcoind

Maybe check if datadir is specified in extra_args in TestNode.start?

laanwj · 2017-12-20T08:37:03Z

Thank you so much for adding tests for this!
utACK be9a13c

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in #11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for #11819 / #1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

maflcko · 2017-12-20T23:18:39Z

Post-merge:

jnewbery · 2018-02-06T21:30:38Z

test/functional/conf_args.py

+        new_data_dir_2 = os.path.join(default_data_dir, 'newdatadir2')
+
+        # Check that using -datadir argument on non-existent directory fails
+        self.nodes[0].datadir = new_data_dir


This is a shame. I don't think individual tests should be reaching into a TestNode object to change the datadir, since that's used by the test framework in various places.

@jnewbery You propose to discard the "old" TestNode and create a fresh one whenever the datadir is changed?

No. The problem here is that it's useful to have a static datadir that files like debug.log and stderr are written to, since that helps with debugging failing scripts.

I agree that it's not a clean solution but it's the only thing I could come up with to make it work, how do you propose otherwise to test the datadir argument?

I think you're right. It's not ideal, but it's as good a way as any to do this.

It might be nice to have a log message at the start of the test warning that not all the node logs will be in the standard location:

self.log.warning("This test uses non-default data directories. Not all node logs will be in the default location.")

but probably not worth a separate PR to add that at this point.

be9a13c Add configuration/argument testing (MeshCollider) Pull request description: Adds a new functional test for testing various command line and configuration file argument interactions, that aren't specific enough to other functionality to be placed in other tests. Currently this tests the error messages for non-existent datadir, which would have caught the bug fixed in bitcoin#11829. It also tests that command line arguments override the ones in the config file. I plan on working on a fix for bitcoin#11819 / bitcoin#1044 and then expanding this test with cases for that. Tree-SHA512: 97aea18c67d331db3ca3d0c99c79267cf012df67fddafc9fac63d392f5c3a6469aa14d93b5865c3bbe561461648d2485367978a77446483b8df53d1916f1c8e8

maflcko reviewed Dec 13, 2017

View reviewed changes

fanquake added the Tests label Dec 13, 2017

maflcko reviewed Dec 13, 2017

View reviewed changes

meshcollider changed the title ~~Add configuration file/argument testing~~ [WiP] Add configuration file/argument testing Dec 14, 2017

meshcollider changed the title ~~[WiP] Add configuration file/argument testing~~ Add configuration file/argument testing Dec 15, 2017

ryanofsky reviewed Dec 18, 2017

View reviewed changes

Add configuration/argument testing

be9a13c

promag reviewed Dec 20, 2017

View reviewed changes

laanwj merged commit be9a13c into bitcoin:master Dec 20, 2017

meshcollider deleted the 201712_datadir_tests branch December 20, 2017 11:31

jnewbery reviewed Feb 6, 2018

View reviewed changes

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add configuration file/argument testing #11883

Add configuration file/argument testing #11883

Uh oh!

Conversation

meshcollider commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Dec 19, 2017

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 20, 2017

Uh oh!

maflcko commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

meshcollider commented Dec 12, 2017 •

edited

Loading

meshcollider Dec 13, 2017 •

edited

Loading