test: Turn rpcauth.py test into functional test #32881

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2507-test-rpcauth-fun changing 7 files +30 −34
  1. maflcko commented at 3:30 pm on July 5, 2025: member

    Currently the rpcauth-test.py is problematic, because:

    • The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. Specifically ConfigParser.
    • The cmake/ci behavior is brittle and can silently fail, as explained in #31476.
    • Outside of ctest, this single test has to be run manually and separately, which is easy to forget.
    • If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests.
    • It is also the only “unit” test written in Python, but not called by the functional test runner.

    Fix all issues by turning it into a functional test.

  2. DrahtBot commented at 3:31 pm on July 5, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32881.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK l0rinc, janb84, w0xlt

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32865 (cmake: Use AUTHOR_WARNING for warnings by fanquake)
    • #32773 (cmake: Create subdirectories in build tree in advance by hebasto)
    • #29247 (CAT in Tapscript (BIP-347) by arminsabouri)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. maflcko renamed this:
    Turn rpcauth.py test into functional test
    test: Turn rpcauth.py test into functional test
    on Jul 5, 2025
  4. DrahtBot added the label Tests on Jul 5, 2025
  5. maflcko force-pushed on Jul 5, 2025
  6. DrahtBot added the label CI failed on Jul 5, 2025
  7. DrahtBot commented at 3:33 pm on July 5, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45404790802 LLM reason (✨ experimental): The CI failure was caused by a lint check that detected a coding style violation (unused import and a spelling error), which caused the linting step to fail.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  8. maflcko force-pushed on Jul 5, 2025
  9. DrahtBot removed the label CI failed on Jul 5, 2025
  10. in test/config.ini.in:7 in fa8a55e116 outdated
    2@@ -3,7 +3,7 @@
    3 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
    4 
    5 # These environment variables are set by the build process and read by
    6-# test/*/test_runner.py and test/util/rpcauth-test.py
    7+# test/*/test_runner.py
    8 
    


    l0rinc commented at 11:47 am on July 8, 2025:

    nit: the line is short enough now to be joined

    0# These environment variables are set by the build process and read by test/*/test_runner.py
    

    maflcko commented at 2:53 pm on July 8, 2025:
    I don’t think it is. At least my IDE (vim) doesn’t join it. Also, the diff is smaller without joining. Going to leave as-is.
  11. in test/config.ini.in:9 in fa8a55e116 outdated
     2@@ -3,7 +3,7 @@
     3 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
     4 
     5 # These environment variables are set by the build process and read by
     6-# test/*/test_runner.py and test/util/rpcauth-test.py
     7+# test/*/test_runner.py
     8 
     9 [environment]
    10 CLIENT_NAME=@CLIENT_NAME@
    


    l0rinc commented at 11:53 am on July 8, 2025:

    note: was wondering who else is using RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py below - ran all functional tests and rpc_users.py (and tool_rpcauth.py of course) are still using it.

    This also demonstrates why the current change makes sense - and that the same env is read successfully and that the test is indeed run automatically now as part of the functional test suite 👍!

  12. in test/functional/tool_rpcauth.py:4 in fa8a55e116 outdated
    3@@ -4,24 +4,18 @@
    4 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
    


    l0rinc commented at 11:55 am on July 8, 2025:
    nit: we might as well update the copyright headers

    maflcko commented at 2:53 pm on July 8, 2025:
    thx, done
  13. in test/functional/tool_rpcauth.py:56 in fa8a55e116 outdated
    53+        suite = unittest.TestLoader().loadTestsFromTestCase(TestRPCAuth)
    54+        result = unittest.TextTestRunner(stream=sys.stdout, verbosity=1, failfast=True).run(suite)
    55+        assert result.wasSuccessful()
    56+
    57+
    58+if __name__ == "__main__":
    


    l0rinc commented at 12:18 pm on July 8, 2025:

    super-duper-nit (to minimize diff and since this version is found 5x more often in the codebase):

    0if __name__ == '__main__':
    

    maflcko commented at 2:53 pm on July 8, 2025:
    black will change those back to ". However, to keep it move-only I’ve changed it back to '.
  14. in test/functional/tool_rpcauth.py:53 in fa8a55e116 outdated
    50+    def run_test(self):
    51+        sys.path.insert(0, os.path.dirname(self.config["environment"]["RPCAUTH"]))
    52+        TestRPCAuth.rpcauth = importlib.import_module("rpcauth")
    53+        suite = unittest.TestLoader().loadTestsFromTestCase(TestRPCAuth)
    54+        result = unittest.TextTestRunner(stream=sys.stdout, verbosity=1, failfast=True).run(suite)
    55+        assert result.wasSuccessful()
    


    l0rinc commented at 12:28 pm on July 8, 2025:
    we would indeed fail here if we had any errors above 👍
  15. in test/functional/tool_rpcauth.py:52 in fa8a55e116 outdated
    36@@ -43,5 +37,21 @@ def test_check_password_hmac(self):
    37 
    


    l0rinc commented at 12:31 pm on July 8, 2025:

    nit: this test seems to duplicate the exact implementation of password_to_hmac - not sure how useful that is.

    But if we keep it, we could simplify the above to:

    0from hashlib import sha256
    1m = hmac.new(salt.encode(), password.encode(), sha256)
    

    (since we just checked above that salt and password are always ASCII)


    maflcko commented at 2:52 pm on July 8, 2025:
    leaving as-is for now, as this is move-only and the code could be changed independently in another pull, if there is need.
  16. in test/functional/tool_rpcauth.py:50 in fa8a55e116 outdated
    47+    def setup_network(self):
    48+        pass
    49+
    50+    def run_test(self):
    51+        sys.path.insert(0, os.path.dirname(self.config["environment"]["RPCAUTH"]))
    52+        TestRPCAuth.rpcauth = importlib.import_module("rpcauth")
    


    l0rinc commented at 12:52 pm on July 8, 2025:

    I don’t like the hacky path insert, googling a bit indicates we could just exec the whole module, something like:

    0spec = spec_from_file_location('rpcauth', self.config['environment']['RPCAUTH'])
    1spec.loader.exec_module(TestRPCAuth.rpcauth := module_from_spec(spec))
    

    maflcko commented at 2:52 pm on July 8, 2025:
    heh, I didn’t know. But I’ll leave this as-is for now, because the logic is just moved. If this was changed, it could make sense to do it repo-wide for all sys.path.insert in a separate pull.
  17. in test/functional/tool_rpcauth.py:17 in fa8a55e116 outdated
    14 import unittest
    15 
    16+from test_framework.test_framework import BitcoinTestFramework
    17+
    18+
    19 class TestRPCAuth(unittest.TestCase):
    


    l0rinc commented at 1:51 pm on July 8, 2025:

    it seems that all other functional tests that have such unit tests are all in test/functional/test_framework instead.

     0 test % grep -r 'unittest.TestCase' .
     1./functional/feature_framework_unit_tests.py:# the output of `git grep unittest.TestCase ./test/functional/test_framework`
     2./functional/test_framework/address.py:class TestFrameworkScript(unittest.TestCase):
     3./functional/test_framework/crypto/ellswift.py:class TestFrameworkEllSwift(unittest.TestCase):
     4./functional/test_framework/crypto/muhash.py:class TestFrameworkMuhash(unittest.TestCase):
     5./functional/test_framework/crypto/poly1305.py:class TestFrameworkPoly1305(unittest.TestCase):
     6./functional/test_framework/crypto/chacha20.py:class TestFrameworkChacha(unittest.TestCase):
     7./functional/test_framework/crypto/ripemd160.py:class TestFrameworkKey(unittest.TestCase):
     8./functional/test_framework/crypto/bip324_cipher.py:class TestFrameworkAEAD(unittest.TestCase):
     9./functional/test_framework/crypto/secp256k1.py:class TestFrameworkSecp256k1(unittest.TestCase):
    10./functional/test_framework/key.py:class TestFrameworkKey(unittest.TestCase):
    11./functional/test_framework/script_util.py:class TestFrameworkScriptUtil(unittest.TestCase):
    12./functional/test_framework/wallet_util.py:class TestFrameworkWalletUtil(unittest.TestCase):
    13./functional/test_framework/segwit_addr.py:class TestFrameworkScript(unittest.TestCase):
    14./functional/test_framework/blocktools.py:class TestFrameworkBlockTools(unittest.TestCase):
    15./functional/test_framework/messages.py:class TestFrameworkScript(unittest.TestCase):
    16./functional/test_framework/compressor.py:class TestFrameworkCompressor(unittest.TestCase):
    17./functional/test_framework/script.py:class TestFrameworkScript(unittest.TestCase):
    18./functional/tool_rpcauth.py:class TestRPCAuth(unittest.TestCase):
    

    But instead of moving the new test there - given that all other tool_* tests are in the same place -, we could change the test structure instead to correspond to the other ones in the same category

    0git ls-files test/functional/tool_*.py
    1test/functional/tool_bitcoin_chainstate.py
    2test/functional/tool_rpcauth.py
    3test/functional/tool_signet_miner.py
    4test/functional/tool_utils.py
    5test/functional/tool_utxo_to_sqlite.py
    6test/functional/tool_wallet.py
    

    I was thinking something like this:

     0#!/usr/bin/env python3
     1# Copyright (c) 2015-present The Bitcoin Core developers
     2# Distributed under the MIT software license, see the accompanying
     3# file COPYING or http://www.opensource.org/licenses/mit-license.php.
     4
     5"""Test share/rpcauth/rpcauth.py
     6"""
     7
     8import hmac
     9import re
    10
    11from hashlib import sha256
    12from importlib.util import spec_from_file_location, module_from_spec
    13from test_framework.test_framework import BitcoinTestFramework
    14
    15
    16class ToolRPCAuthTest(BitcoinTestFramework):
    17    def set_test_params(self):
    18        self.num_nodes = 0  # No node/datadir needed
    19
    20    def setup_network(self):
    21        pass  # nothing to start
    22
    23    def run_test(self):
    24        spec = spec_from_file_location('rpcauth', self.config['environment']['RPCAUTH'])
    25        spec.loader.exec_module(rpcauth := module_from_spec(spec))
    26
    27        self.log.info("Verify generate_salt() returns hex of correct length")
    28        for i in range(16, 32 + 1):
    29            assert len(rpcauth.generate_salt(i)) == i * 2
    30
    31        self.log.info("Test that generated passwords only consist of urlsafe characters")
    32        assert re.fullmatch(r"[-\w]+", rpcauth.generate_password(), flags=re.ASCII)
    33
    34        self.log.info("Verify password_to_hmac() matches SHA-256 HMAC")
    35        salt = rpcauth.generate_salt(16)
    36        pwd = rpcauth.generate_password()
    37        expected = hmac.new(salt.encode(), pwd.encode(), sha256).hexdigest()
    38        assert rpcauth.password_to_hmac(salt, pwd) == expected
    39
    40
    41if __name__ == '__main__':
    42    ToolRPCAuthTest(__file__).main()
    

    maflcko commented at 2:52 pm on July 8, 2025:
    yeah, right. Probably best to drop the unittest import here to make review easier. Did that, but a bit different from your suggestion.
  18. l0rinc approved
  19. l0rinc commented at 2:19 pm on July 8, 2025: contributor

    ACK fa8a55e116ba925ce1791605a3ac55bc005f88bf

    This is the continuation of the migration efforts from #32697. It makes sense to unify this with the rest of the tool tests - though it differs from the others in structure, please see my suggestion for how we could make it more like the other functional tool tests.

  20. Turn rpcauth.py test into functional test fa4d68cf97
  21. maflcko force-pushed on Jul 8, 2025
  22. l0rinc commented at 3:57 pm on July 8, 2025: contributor
    ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62
  23. janb84 commented at 6:05 pm on July 8, 2025: contributor

    LGTM ACK fa4d68cf97b6d77dbb559facbc425376e2c51f62

    PR turns rpcauth.py test into functional test, this looks like a continuation of the migration efforts of #32697. Seems like a logical step and it simplifies the test/cmake setup.

    • build & tested ✅
    • code reviewed ✅
    0rpc_whitelist.py                                       | ✓ Passed  | 2 s
    1tool_rpcauth.py                                        | ✓ Passed  | 0 s
    2tool_signet_miner.py                                   | ✓ Passed  | 5 s
    
  24. fanquake merged this on Jul 10, 2025
  25. fanquake closed this on Jul 10, 2025

  26. maflcko deleted the branch on Jul 10, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-12 09:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me