test: Add rpcauth pair that generated by rpcauth.py #13024

ken2812221 commented at 9:40 PM on April 18, 2018: contributor

This PR adds a rpcauth pair that is randomly generated. Also checks that rpcauth.py works fine. Resolve #12995

ken2812221 force-pushed on Apr 18, 2018

fanquake added the label Tests on Apr 18, 2018

ken2812221 commented at 10:43 PM on April 18, 2018: contributor

Can this resolve #12995?

ken2812221 force-pushed on Apr 19, 2018

laanwj commented at 1:01 PM on April 19, 2018: member

Can this resolve #12995?

Yes - looks good to me

in test/functional/rpc_users.py:26 in 45a033f4f4 outdated

  21 |  
  22 |  class HTTPBasicsTest(BitcoinTestFramework):
  23 |      def set_test_params(self):
  24 |          self.num_nodes = 2
  25 | +        self.user = ''.join(SystemRandom().choice(string.ascii_letters + string.digits) for _ in range(10))
  26 | +        p = subprocess.Popen([os.path.join(os.path.dirname(os.path.abspath(__file__)),'..','..','share','rpcauth','rpcauth.py'), self.user], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)

MarcoFalke commented at 1:19 PM on April 19, 2018:

nit: Could give the executable a name

gen_rpcauth = os.path.join(...
p = subprocess.Popen([gen_rpcauth, self.user], ...

in test/functional/rpc_users.py:29 in 45a033f4f4 outdated

  24 |          self.num_nodes = 2
  25 | +        self.user = ''.join(SystemRandom().choice(string.ascii_letters + string.digits) for _ in range(10))
  26 | +        p = subprocess.Popen([os.path.join(os.path.dirname(os.path.abspath(__file__)),'..','..','share','rpcauth','rpcauth.py'), self.user], stdout=subprocess.PIPE, stderr=subprocess.PIPE, stdin=subprocess.PIPE, universal_newlines=True)
  27 | +        lines = p.stdout.read().splitlines()
  28 | +        self.auth = lines[1]
  29 | +        self.password = lines[3]

MarcoFalke commented at 1:20 PM on April 19, 2018:

nit: set_test_params is mostly used to overwrite test params, not set member variables. I think this hunk can go in setup_chain.

in test/functional/rpc_users.py:138 in 45a033f4f4 outdated

 122 | @@ -114,6 +123,28 @@ def run_test(self):
 123 |          assert_equal(resp.status, 401)
 124 |          conn.close()
 125 |  
 126 | +        #Correct for randomly generated user

MarcoFalke commented at 1:21 PM on April 19, 2018:

nit: Could use self.log.info('Correct...') instead of a comment, which is not shown in the debug log and makes debugging harder in case of a failure.

in test/functional/rpc_users.py:150 in 45a033f4f4 outdated

 132 | +        conn.request('POST', '/', '{"method": "getbestblockhash"}', headers)
 133 | +        resp = conn.getresponse()
 134 | +        assert_equal(resp.status, 200)
 135 | +        conn.close()
 136 | +
 137 | +        #Wrong password for randomly generated user

MarcoFalke commented at 1:21 PM on April 19, 2018:

Same

MarcoFalke approved

MarcoFalke commented at 1:22 PM on April 19, 2018: member

utACK. Some nits

ken2812221 force-pushed on Apr 19, 2018

ken2812221 commented at 1:48 PM on April 19, 2018: contributor

@MarcoFalke Done.

cdecker commented at 3:56 PM on April 19, 2018: contributor

ACK

jnewbery commented at 7:28 PM on April 19, 2018: member

I've just skimmed the code, but I'm NACKish on this. I don't think we should be adding dependencies to the functional tests (even if they're internal project dependencies).

Better to just have a unit test for rpcauth.py.

MarcoFalke commented at 9:02 PM on April 19, 2018: member

@jnewbery Mind to elaborate on that. Unlike the unit tests, which are shipped in a single executable, the functional tests are only used by developers, who have all the project files.

Adding a python unit test to rpcauth would add yet another test target that is run by no one (execpt by travis, if we add it to the yaml). And the rpcauth unit test would need to borrow a ton from the functional tests anyway... I don't see the advantage.

jnewbery commented at 9:12 PM on April 19, 2018: member

See my comment in #12995. I don't think it's good practice to have functional tests calling out to random scripts in the share directory, and certainly isn't a good idea to hard code the relative path to that directory.

MarcoFalke commented at 10:52 PM on April 19, 2018: member

certainly isn't a good idea to hard code the relative path to that directory

That could easily be fixed by adjusting configure.ac to create an appropriate link in the test directory. (And read the BUILDDIR from our confg.ini)

ken2812221 force-pushed on Apr 20, 2018

ken2812221 commented at 1:08 AM on April 20, 2018: contributor

~~I modified configure.ac, it can generate a symlink during ./configure~~ Now I just add rpcauth path to config.ini

ken2812221 force-pushed on Apr 21, 2018

laanwj commented at 6:16 AM on April 21, 2018: member

I don't think it's good practice to have functional tests calling out to random scripts in the share directory

It also calls out to "random executables" in the src directory. It's a good default assumption.

However, I do agree that the path ideally needs to be override-able with an environment variable, like BITCOIND and BITCOINCLI are.

Better to just have a unit test for rpcauth.py.

This doesn't exclude also adding a unit test later. But IMO the integration test as it is makes sense, as this is how the script is supposed to be used with bitcoind (this will find both problems with rpcauth.py and with bitcoind's authentication based on those tokens).

ken2812221 renamed this:
~~test: Add rpcauth pair that generated by rpcauth~~
test: Add rpcauth pair that generated by rpcauth.py
on Apr 22, 2018

jnewbery commented at 10:14 PM on April 22, 2018: member

It also calls out to "random executables" in the src directory.

No - it calls out specifically to the bitcoind, bitcoin-qt and bitcoin-cli executables, which is what the functional test framework should be testing in my opinion. I don't think expanding the functional test framework to also test scripts in the source directory is a good idea. We already drive far too much testing through the functional test framework.

Edit: I won't stand in the way if everyone else thinks this is a good idea, but using the functional test framework to also test python scripts just seems like the wrong approach to me.

test: Add rpcauth pair that generated by rpcauth 8b8032e283

in test/functional/rpc_users.py:38 in 6be22e7db3 outdated

  30 | @@ -27,9 +31,21 @@ def setup_chain(self):
  31 |          rpcauth2 = "rpcauth=rt2:f8607b1a88861fac29dfccf9b52ff9f$ff36a0c23c8c62b4846112e50fa888416e94c17bfd4c42f88fd8f55ec6a3137e"
  32 |          rpcuser = "rpcuser=rpcuser💻"
  33 |          rpcpassword = "rpcpassword=rpcpassword🔑"
  34 | +
  35 | +        self.user = ''.join(SystemRandom().choice(string.ascii_letters + string.digits) for _ in range(10))
  36 | +        config = configparser.ConfigParser()
  37 | +        configfile = self.options.configfile
  38 | +        config.read_file(open(configfile))

MarcoFalke commented at 10:22 PM on April 22, 2018:

nit: Just make this open(self.options.configfile)

ken2812221 force-pushed on Apr 22, 2018

laanwj commented at 5:29 AM on April 23, 2018: member

Edit: I won't stand in the way if everyone else thinks this is a good idea, but using the functional test framework to also test python scripts just seems like the wrong approach to me.

I think the functional test framework should be able to test everything that is supposed to be used by the user. So probably not developer/maintainer scripts such as github-merge.py, but yes this script is something of a grey area. (But just that it is written in python is not a valid argument, IMO, to not test it in the framework)

(in similar vein I've also wondered before why this script isn't installed on make install)

laanwj commented at 6:36 AM on April 24, 2018: member

Tested ACK 8b8032e283ae1dff00de71e2d9d28c0df626fc23, also verified that it works for out-of-tree builds.

MarcoFalke commented at 10:34 AM on April 24, 2018: member

utACK 8b8032e

laanwj merged this on Apr 24, 2018

laanwj closed this on Apr 24, 2018

laanwj referenced this in commit 018c7e5dad on Apr 24, 2018

in test/functional/rpc_users.py:192 in 8b8032e283

 188 | @@ -143,6 +189,7 @@ def run_test(self):
 189 |          conn.close()
 190 |  
 191 |          #Wrong password for rpcuser
 192 | +        self.log.info('Wrong...')

kanzure commented at 1:09 PM on April 24, 2018:

this is not very descriptive ("wrong")

ken2812221 deleted the branch on Apr 24, 2018

PastaPastaPasta referenced this in commit da37f120c6 on Jul 19, 2020

PastaPastaPasta referenced this in commit d57503e469 on Jul 22, 2020

MarcoFalke locked this on Sep 8, 2021