ci: switch back to executing functional tests with powershell #29581

1. 
   m3dwards commented at 4:04 pm on March 6, 2024: contributor

   In this PR: #29535 it was noticed that the functional test runner was skipping running the tests with the message of Test '' not found in full test list

   I believe that this was caused by Powershell passing a blank TEST_RUNNER_EXTRA environment variable as an empty flag to the test_runner.py script which then tries to find a test to run with a blank name.

   This PR switches the functional tests back to using Powershell but instead of passing an empty argument, constructs a command string and then executes that command. It also prints the powershell version to help diagnose any issues in the future (not that this issue was related to powershell version).

   I believe it’s best to switch back to powershell rather than stick with cmd, even though it currently works, as this is what I would expect the majority of users and windows developers would use.

   ---

   Some extra notes:

   • Run with --extended takes ~ 28 minutes
   • Run without --extended takes ~ 15 minutes
   • The mac job appears to never run with --extended
2. ci: add print of powershell version to win64 job 115c283516
3. ci: switch win64 fun tests back to powershell

   When env var TEST_RUNNER_EXTRA was empty it was still being passed to test runner as an empty arguemnt which would then try and run a test with an empty name.

   c4c907c492
4. 
   DrahtBot commented at 4:05 pm on March 6, 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. A summary of reviews will appear here.

5. DrahtBot added the label Tests on Mar 6, 2024
6. glozow requested review from hebasto on Mar 6, 2024
7. glozow requested review from fanquake on Mar 6, 2024
8. 
   fanquake commented at 9:53 am on March 7, 2024: member

   Seems fine, however I still don’t understand what/how this broke in the first place. Givne the timings you posted above, I’m somewhat inclined for us to just always run with the extended tests, given the only reason this distinction existed was because the runtimes used to much longer. However, ~30 minutes for a run with extended tests seems well within the bounds of all the other CIs, and Native Windows is not a priority-to-finish for developers.
9. 
   m3dwards commented at 10:27 am on March 7, 2024: contributor

   I think it must have been a python or more likely powershell update that treats empty arguments differently?

   You are right something must have changed as here is an old job that worked back then but would not work now (https://github.com/bitcoin/bitcoin/actions/runs/7531813057/job/20501171129).

10. 
    hebasto commented at 4:48 pm on March 7, 2024: member

    I believe it’s best to switch back to powershell rather than stick with cmd, even though it currently works, as this is what I would expect the majority of users and windows developers would use. @sipsorcery What do you think?

11. 
    hebasto commented at 6:51 pm on March 7, 2024: member

    … I still don’t understand what/how this broke in the first place.

    The breaking change was introduced in PowerShell 7.3:

    Windows and Linux/macOS have fundamental differences in how native command arguments are handled specifically when quotes are involved. We added a new feature $PSNativeCommandArgumentPassing to control how PowerShell passes arguments to native commands. The default behavior for Windows and Linux/macOS should work as most users expect in their respective environments.

    tl;dr: Empty strings are preserved as arguments now.

    The CI was broken due to the PowerShell upgrade from 7.2.18 to 7.4.1.

12. 
    sipsorcery commented at 8:12 pm on March 7, 2024: contributor

    I believe it’s best to switch back to powershell rather than stick with cmd, even though it currently works, as this is what I would expect the majority of users and windows developers would use.

    @sipsorcery What do you think?

    No strong preference. I use DOS or PowerShell depending on the task at hand.

    I do lean slightly towards DOS as it’s simpler. Perhaps the original issue is a proof in point. DOS is unlikely to ever change and break the tests whereas a future PowerShell update might.

13. 
    m3dwards commented at 10:13 am on March 8, 2024: contributor

    @hebasto great find!

    Happy to stick with cmd.exe which I agree is less likely to have breaking changes in the future.

    As per @fanquake’s suggestion, shall we keep it simple and just run extended always?

14. 
    hebasto commented at 10:15 am on March 8, 2024: member

    Happy to stick with cmd.exe which I agree is less likely to have breaking changes in the future.

    I agree. Besides, we also use the cmd shell in the “Build” step.

15. 
    hebasto commented at 10:21 am on March 8, 2024: member

    As per @fanquake’s suggestion, shall we keep it simple and just run extended always?

    See the previous discussions in:

    • #28173 (review)
    • https://github.com/bitcoin/bitcoin/pull/28360
16. 
    fanquake commented at 10:29 am on March 8, 2024: member

    See the previous discussions in:

    Can you point to something specific? Not sure that’s relevant given the change in runtimes, and now they no-longer seem to be randomly failing so often.

17. 
    hebasto commented at 10:39 am on March 8, 2024: member

    See the previous discussions in:

    Can you point to something specific? Not sure that’s relevant given the change in runtimes, and now they no-longer seem to be randomly failing so often.

    However, ~30 minutes for a run with extended tests seems well within the bounds of all the other CIs, and Native Windows is not a priority-to-finish for developers.

    The number of CI Windows runners is limited. Extending runtime will decrease the availability of runners for all developers. Besides, the developer, who is interested in extended functional tests for their PRs, might run GHA CI in their own repo.

18. 
    m3dwards commented at 11:53 am on March 11, 2024: contributor

    Not much appetite to switch back to powershell so closing this PR.
19. m3dwards closed this on Mar 11, 2024

    ---

20. fanquake referenced this in commit d14c7286b6 on Mar 12, 2024
21. bitcoin locked this on Mar 11, 2025

Contributors
m3dwards DrahtBot fanquake hebasto sipsorcery

Review Requested
fanquake hebasto

Labels
Tests