test: allow all functional tests to be run or skipped with –usecli #32290

pull mzumsande wants to merge 11 commits into bitcoin:master from mzumsande:202505_fix_usecli changing 40 files +94 −80
  1. mzumsande commented at 8:50 pm on April 16, 2025: contributor

    Fixes #32264

    I looked into all current failures listed in the issue, as well all tests that are already disabled for the cli with self.supports_cli = False. There are several reasons why existing tests fail with --usecli on many systems, the most important ones are:

    • Most common reason is that the test executes a RPC call with a large arg that exceeds MAX_ARG_STRLEN of the OS, which is usually 128kb on linux: This is fixed by using -stdin for these large calls (idea by 0xB10C)
    • they test specifically the rpc interface - nothing to do there except disabling.
    • Some functional test submit wrong types to params on purpose to test the error message (which is different when using the cli) - deactivated these specific subtests locally for the cli when there is just one or two of them, deactivated the entire tests when there are more spots
    • When python sets None for an arg, the cli converts this to ’null’ in arg_to_cli. This is fine e.g. for boolean args, but doesn’t work for strings where it’s interpreted as the string ’null’. Bypass this for named args by not including args in case the value is None for the cli is used (it’s effectively the same as leaving the optional arg out).
    • the height_or_hash param used in some RPC needs to be converted to a JSON (effectively adding full quotes).
    • Some tests were marked with self.supports_cli = False in the past but run fine on master today - enabled those.

    In total, this PR fixes all tests that fail on master and reduces the number of tests that are deactivated (self.supports_cli = False) from 40 to 21. It also adds --usecli to one CI job (multiprocess, i686, DEBUG) to detect regressions.

  2. DrahtBot commented at 8:50 pm on April 16, 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/32290.

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    Conflicts

    No conflicts as of last run.

  3. DrahtBot added the label Tests on Apr 16, 2025
  4. mzumsande marked this as a draft on Apr 16, 2025
  5. mzumsande force-pushed on Apr 16, 2025
  6. mzumsande force-pushed on Apr 16, 2025
  7. DrahtBot added the label CI failed on Apr 16, 2025
  8. DrahtBot commented at 11:01 pm on April 16, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40687372455

    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.

  9. mzumsande force-pushed on Apr 16, 2025
  10. DrahtBot removed the label CI failed on Apr 17, 2025
  11. in test/functional/test_framework/util.py:607 in 1c2ea56030 outdated
    602@@ -603,3 +603,8 @@ def sync_txindex(test_framework, node):
    603     sync_start = int(time.time())
    604     test_framework.wait_until(lambda: node.getindexinfo("txindex")["txindex"]["synced"])
    605     test_framework.log.debug(f"Synced in {time.time() - sync_start} seconds")
    606+
    607+def add_quotes_for_cli(cli, text):
    


    maflcko commented at 8:12 am on April 17, 2025:

    q in 1c2ea560300f57e6b5ed75dee2597ca55dd76c6f: Would it not be easier if this was auto-detected? That is, if the type is string, would it not be better to internally (before dispatching to the cli) add quotes to all strings?

    This probably means that numbers must be passed as int, or Decimal on the python side, which seems fine.


    mzumsande commented at 5:33 pm on April 17, 2025:
    Hmm, this is only an issue because hash_or_height expects a JSON object, not a string (and a text without quotes is no valid JSON object), so regular string args don’t need this. I think the current naming is confusing, maybe renaming the function “convert_to_json_for_cli” and calling json.dumps(text) inside instead of adding quotes (in case the cli is used) would be better to convey why this is done? Or is there a better solution?

    mzumsande commented at 9:53 pm on April 30, 2025:
    renamed to convert_to_json_for_cli
  12. fanquake commented at 9:02 am on April 17, 2025: member
    If this ends up fixing all issues, should add --usecli to atleast one CI, so we catch regressions?
  13. mzumsande commented at 9:57 pm on April 17, 2025: contributor

    If this ends up fixing all issues, should add –usecli to atleast one CI, so we catch regressions?

    Yes - it’d be a bit of a waste of a CI job if we end up disabling too many of the tests with self.supports_cli = False but depending on how many can be fixed (I’m currently still going through all the existing disabled tests) that would make sense to me.

  14. mzumsande force-pushed on Apr 18, 2025
  15. mzumsande marked this as ready for review on Apr 21, 2025
  16. mzumsande force-pushed on Apr 24, 2025
  17. DrahtBot added the label Needs rebase on Apr 25, 2025
  18. mzumsande force-pushed on Apr 25, 2025
  19. DrahtBot removed the label Needs rebase on Apr 25, 2025
  20. mzumsande force-pushed on Apr 30, 2025
  21. mzumsande commented at 9:55 pm on April 30, 2025: contributor

    If this ends up fixing all issues, should add --usecli to atleast one CI, so we catch regressions?

    I have added --usecli to one CI job now (multiprocess, i686, DEBUG)

  22. test: Enable various tests for usage with cli
    These tests run successfully on current master without any changes.
    a0e595bc51
  23. test: use -stdin for large rpc commands
    Because of the MAX_ARG_STRLEN limit (128kb on most systems)
    for args, these would usually fail. As a workaround, use
    -stdin for these large calls. Idea by 0xB10C.
    5cf8f01dc0
  24. test: enable functional tests with large rpc args for cli
    Also, the following tests (for which self.supports_cli = False was not
    set) will now work with --usecli:
    
    feature_fastprune.py
    feature_fee_estimation.py
    feature_reindex_readonly.py
    feature_taproot.py
    mempool_package_rbf.py
    p2p_net_deadlock.py
    p2p_tx_download.py
    rpc_packages.py
    f66de4e94d
  25. test: remove unnecessary .rpc
    The tests will run with --usecli without it
    4c651c7d4d
  26. test: make rpc_psbt.py usable with --usecli
    The psbt string would include a "=" sign, which would
    make the cli interpret this as a named argument.
    Fix this by making it an actual named arg with the
    correct name.
    ffe5d537f6
  27. test: convert tuple to json for cli
    This makes it possible to run rpc_createmultisig.py with --usecli.
    d1e051a3c7
  28. test: Don't send empty named args with cli
    If python passed None for an optional (i.e. 'null' is
    sent), this will lead to the arg being interpreted as not
    provided by bitcoind - except for string args, for which the arg is
    interpreted as as 'null' string. Bypass this by not sending
    named args to bitcoin-cli - so that the default value will
    actually be used. The issue still persists for positional args,
    but the existing test "wallet_labels.py" affected by this can
    be changed to use named args instead.
    
    This also makes wallet_labels.py --usecli no longer fail.
    84893f5a85
  29. test: add function to convert to json for height_or_hash params
    This is necessary for bitcoin-cli because a string without quotes
    is not a valid json.
    959dc33283
  30. test: skip subtests that check for wrong types with cli
    The error messages returned for wrong type differ betwen
    using rpc or cli - skip these subtests where they happen.
    bc52451ac3
  31. test: Disable several (sub)tests with cli
    Reason for each test:
    rpc_whitelist.py: Relies on direct RPC calls
    wallet_encryption.py: Null characters cannot be passed to suprocess.Popen
    wallet_fundrawtransaction.py: multiple checks for wrong types, which have different error messages with cli
    wallet_send.py: multiple checks for wrong types
    38352c3aa0
  32. ci: use --usecli in one of the CI jobs
    This helps avoid regressions (changes to functional tests
    that are not compatible with -usecli).
    7c13240d7b
  33. mzumsande force-pushed on May 2, 2025
  34. mzumsande commented at 7:42 pm on May 2, 2025: contributor
    Also added a commmit to fix the too-large args (https://github.com/bitcoin/bitcoin/pull/32399#issuecomment-2846837427).

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-05-05 15:12 UTC

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