ci: parse TEST_RUNNER_EXTRA into an array #30244

1. 
   m3dwards commented at 12:20 pm on June 7, 2024: contributor

   While working on CI I wanted to disable some functional tests so I used the TEST_RUNNER_EXTRA var. The problem I had was tests that have flags such as rpc_bind.py --ipv6 must be passed in quotes otherwise the --ipv6 portion will be considered an argument to test_runner.py rather than a test name.

   This change allows proper parsing of quotes and complex values such as:

   0TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
   

   Update:

   While testing this it was noticed that test_runner.py when given --exclude "rpc_bind.py --ipv6" will exclude all rpc_bind.py tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. --exclude rpc_bind.py will exclude all three variants and --exclude rpc_bind --ipv6 will only exclude the IPV6 variant.

2. 
   DrahtBot commented at 12:20 pm on June 7, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	hebasto, maflcko, achow101

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

3. DrahtBot added the label Tests on Jun 7, 2024
4. in ci/test/03_test_script.sh:163 in b9c0b79ab7 outdated

   160@@ -161,7 +161,9 @@ fi
   161 
   162 if [ "$RUN_FUNCTIONAL_TESTS" = "true" ]; then
   163   # shellcheck disable=SC2086
   

   ---

   ---

   

   maflcko commented at 12:59 pm on June 7, 2024:

   Is this still needed?

   ---

   

   m3dwards commented at 3:14 pm on June 7, 2024:

   The comment? I’m not sure

   ---

   

   maflcko commented at 3:20 pm on June 7, 2024:

   You can test by removing it and pushing, then observing the colour of the lint task after 3 minutes.
5. maflcko approved
6. 
   maflcko commented at 1:01 pm on June 7, 2024: member

   I can’t review this, because it is written in bash, but testing seems to indicate that this works as intended for some reason.

   ACK b9c0b79ab7a0d4dafd5386a78f4fd747236c8f88

7. 
   hebasto commented at 1:09 pm on June 7, 2024: member

   This change allows proper parsing of quotes and complex values such as:

   0TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6, feature_proxy.py"'
   

   Does such an example work on Windows in Command Prompt and PowerShell?

8. 
   maflcko commented at 1:11 pm on June 7, 2024: member

   Does such an example work on Windows in Command Prompt and PowerShell?

   PowerShell and Command Prompt are not supported by ./ci/. Only bash, according to https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally

9. 
   hebasto commented at 1:13 pm on June 7, 2024: member

   Does such an example work on Windows in Command Prompt and PowerShell?

   PowerShell and Command Prompt are not supported by ./ci/. Only bash, according to https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally

   I’m aware of that. I’m asking about TEST_RUNNER_EXTRA variable content given in the example.

10. 
    m3dwards commented at 1:17 pm on June 7, 2024: contributor

    Does such an example work on Windows in Command Prompt and PowerShell?

    I will test it.

11. m3dwards force-pushed on Jun 7, 2024
12. 
    m3dwards commented at 3:16 pm on June 7, 2024: contributor

    Force pushed but only changed the commit description as it was a bad example with an extra space
13. 
    maflcko commented at 3:18 pm on June 7, 2024: member

    reACK 3d976000afccac4e89931496f23cfe8593daeb75
14. 
    m3dwards commented at 9:34 pm on June 7, 2024: contributor

    Does such an example work on Windows in Command Prompt and PowerShell?

    I can confirm that TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"' works with cmd as defined in .github/workflows/ci.yml. Not tested Powershell but I don’t believe we use this with Powershell.

    Windows CI run using that value

15. 
    hebasto commented at 10:23 am on June 8, 2024: member

    Tested 3d976000afccac4e89931496f23cfe8593daeb75 on Ubuntu 24.04.

    Passing --exclude "rpc_bind.py --ipv6" excludes not only the specified test but also rpc_bind.py --ipv4 and rpc_bind.py --nonloopback.

16. m3dwards force-pushed on Jun 8, 2024
17. m3dwards marked this as a draft on Jun 8, 2024
18. 
    m3dwards commented at 4:22 pm on June 8, 2024: contributor

    Converting back to draft to see if I can fix the bug @hebasto found.
19. DrahtBot added the label CI failed on Jun 8, 2024
20. 
    DrahtBot commented at 5:34 pm on June 8, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is 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.

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

    Debug: https://github.com/bitcoin/bitcoin/runs/25977109047

21. in ci/test/03_test_script.sh:197 in 80f016badd outdated

    194@@ -194,6 +195,5 @@ if [ "${RUN_TIDY}" = "true" ]; then
    195 fi
    196 
    197 if [ "$RUN_FUZZ_TESTS" = "true" ]; then
    198-  # shellcheck disable=SC2086
    

    ---

    ---

    

    maflcko commented at 7:51 am on June 9, 2024:

    This is still needed. You didn’t change the fuzz test runner invocation, only the functional test runner invocation.

    ---

    

    maflcko commented at 8:01 am on June 9, 2024:

    You will have to remove this diff hunk, or apply the same approach to the fuzz test runner, as you did to the functional test runner.
22. 
    maflcko commented at 8:02 am on June 9, 2024: member

    Passing --exclude "rpc_bind.py --ipv6" excludes not only the specified test but also rpc_bind.py --ipv4 and rpc_bind.py --nonloopback.

    This is the existing behavior on current master and this pull request. I don’t think it was ever supported. Though, I guess this means this pull request (in its current form) isn’t needed, then.

23. test: allow excluding func test by name and arg

    Can now specify test_runner.py --exclude "rpc_bind.py --ipv6" and have only that test variant excluded

    c4762b0aa0
24. ci: parse TEST_RUNNER_EXTRA into an array

    Allows for parsing quotes and multiple arguments such as TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'

    8131bf7483
25. m3dwards force-pushed on Jun 12, 2024
26. 
    m3dwards commented at 5:36 pm on June 12, 2024: contributor

    Updated test_runner.py so that:

    --exclude "rpc_bind.py --ipv6 will only exclude that variant. --exclude rpc_bind.py will exclude all variants

    Green test run with TEST_RUNNER_EXTRA: --exclude "rpc_bind.py --ipv6, feature_proxy.py": https://github.com/m3dwards/bitcoin/actions/runs/9485948819/job/26139127116. Note that rpc_bind.py --ipv4 and rpc_bind.py --nonloopback were both run but rpc_bind.py --ipv6 and feature_proxy.py did not run.

    Spaces between test names now also works.

27. m3dwards marked this as ready for review on Jun 12, 2024
28. DrahtBot removed the label CI failed on Jun 13, 2024
29. hebasto approved
30. 
    hebasto commented at 9:37 am on June 17, 2024: member

    ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f, tested on Ubuntu 23.10 and Windows 11.

    In the command prompt on Windows, the following commands work as expected:

    0>set EXTRA=--exclude "rpc_bind.py --ipv6, feature_proxy.py"
    1>py -3 test\functional\test_runner.py %EXTRA%
    

    which is sufficient for our CI if needed.

31. DrahtBot requested review from maflcko on Jun 17, 2024
32. DrahtBot added the label CI failed on Jun 17, 2024
33. DrahtBot removed the label CI failed on Jun 19, 2024
34. 
    maflcko commented at 8:01 pm on July 2, 2024: member

    ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f
35. 
    achow101 commented at 7:49 pm on September 4, 2024: member

    ACK 8131bf7483c0ea10d3573c9f2e977d19d8569b7f
36. achow101 merged this on Sep 4, 2024
37. achow101 closed this on Sep 4, 2024

    ---

38. 
    maflcko commented at 3:34 pm on September 11, 2024: member

    Actually, this is wrong. Sorry for the wrong review.

    This breaks all existing configs:

    0$ git grep '\-exclude' ci/test/00_setup_env*
    1ci/test/00_setup_env_native_previous_releases.sh:export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_dbcrash"  # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash
    2ci/test/00_setup_env_native_valgrind.sh:export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra"  # feature_init excluded for now, see [#30011](/bitcoin-bitcoin/30011/) ; bind tests excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
    3ci/test/00_setup_env_s390x.sh:export TEST_RUNNER_EXTRA="--exclude rpc_bind,feature_bind_extra"  # Excluded for now, see [#17765 (comment)](/bitcoin-bitcoin/17765/#issuecomment-602068547)
    

    For example, the dbcrash test is no longer excluded (and thus run, and may contribute to the CI timeouts observed).

    0+ '[' true = true ']'
    1+ eval 'TEST_RUNNER_EXTRA=(--previous-releases --coverage --extended --exclude feature_dbcrash)'
    2++ TEST_RUNNER_EXTRA=(--previous-releases --coverage --extended --exclude feature_dbcrash)
    3+ LD_LIBRARY_PATH=/ci_container_base/depends/x86_64-pc-linux-gnu/lib
    4+ test/functional/test_runner.py --ci -j10 --tmpdirprefix /ci_container_base/ci/scratch/test_runner/ --ansi --combinedlogslen=99999999 --timeout-factor=40 --previous-releases --coverage --extended --exclude feature_dbcrash --quiet --failfast
    5WARNING! Test 'feature_dbcrash' not found in current test list.
    6...............................................................................................................................
    7                                                                                                                               
    8........
    

    https://cirrus-ci.com/task/5507237274189824?logs=ci#L3985

39. 
    m3dwards commented at 4:20 pm on September 11, 2024: contributor

    @maflcko no apology needed, apologies I introduced a bug!

    I will work out what’s going on and propose a fix PR ASAP.

    Thanks for the logs.

40. m3dwards deleted the branch on Sep 11, 2024

Contributors
m3dwards DrahtBot maflcko hebasto achow101

Review Requested
maflcko

Labels
Tests