feature: Use S2N_FAST_INTEG_TESTS to run pytest in parallel under nix #4368
Conversation
| @@ -551,6 +551,10 @@ if (BUILD_TESTING) | |||
| if (S2N_INTEG_TESTS) | |||
| find_package (Python3 COMPONENTS Interpreter Development) | |||
| file(GLOB integv2_test_files "${PROJECT_SOURCE_DIR}/tests/integrationv2/test_*.py") | |||
| set(N 1) | |||
| if (S2N_FAST_INTEG_TESTS) | |||
There was a problem hiding this comment.
Why is this behind a environment variable? Shouldn't it be on by default?
There was a problem hiding this comment.
It's a CMAKE flag that's off by default on https://github.com/aws/s2n-tls/blob/main/CMakeLists.txt#L35 ; since the non-nix integration tests are run with Make, we could flip it to ON, but I'm trying to not to change the behavior too much. Either way, it might be worth adding an else case here.
There was a problem hiding this comment.
I think I'd still like it to be on by default. Since having it off by default just makes it more difficult for people to select the right thing. But at least having it as an option is better than not having it as an option.
There was a problem hiding this comment.
Flipped to on by default.
| @@ -551,6 +551,10 @@ if (BUILD_TESTING) | |||
| if (S2N_INTEG_TESTS) | |||
| find_package (Python3 COMPONENTS Interpreter Development) | |||
| file(GLOB integv2_test_files "${PROJECT_SOURCE_DIR}/tests/integrationv2/test_*.py") | |||
| set(N 1) | |||
| if (S2N_FAST_INTEG_TESTS) | |||
There was a problem hiding this comment.
I think I'd still like it to be on by default. Since having it off by default just makes it more difficult for people to select the right thing. But at least having it as an option is better than not having it as an option.
Resolved issues:
#2469
Description of changes:
While we don't have a root-cause, there is anecdata that nix (newer toolchain) has a better record of running more than 2 integration test runners in parallel. The original PR for nix contained a CMAKE flag to enable this, but it was removed.
This PR adds it back in and turns on
S2N_FAST_INTEG_TESTSby default, which currently only affects nix. The regular integration jobs are built with Make/tox and are pinned at 2 workersExample use with it enabled by default (basically how CI works now):
Falling back to 1 xdist worker in nix:
With "auto" as the
pytest xdistrunner count, the current batch of integration tests finish in 64 minutes vs. 2 hours with 4 workers sample - both only having 2 "waves".Call-outs:
The CMake section updated is specifically for Nix.
Not all of the current integration tests run successfully under nix.
Using a default xdist value of 1 could take upwards of 6 hours to finish an integration test run.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Locally and ad-hoc CodeBuild. CI run just verifies that no Makefile based integ tests are broken.
Is this a refactor change? no
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.