tests: local test speed optimizations, add cargo hack feature powerset #176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpu
commented
Sep 13, 2023
Codecov Report
@@ Coverage Diff @@
## main #176 +/- ##
==========================================
- Coverage 96.69% 96.31% -0.38%
==========================================
Files 17 17
Lines 4506 4510 +4
==========================================
- Hits 4357 4344 -13
- Misses 149 166 +17
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
djc
approved these changes
Sep 13, 2023
ctz
reviewed
Sep 13, 2023
The `test_too_many_path_calls` unit test artificially inflates the budget for signature validation operations so that it can easily force quadratic path building runtime, up to the default `build_chain_calls` budget. As a result this test takes a long time relative to our other unit tests: it completes in ~3s while the entire project test suite without this test and the bettertls tests in 0.6s. This commit adjusts the test to use a lower-than-default `build_chain_calls` budget. This lets us test the mechanics of the budget are working as intended, without having to expend a lot of runtime to run up the budget to the large default.
The Netflix bettertls test suite for pathbuilding and name constraint validation take an outsized amount of runtime compared to our other tests. These tests can take ~8s locally and the entire project test suite with these tests ignored can be run in 0.6s. This commit adds an `#[ignore]` flag so it won't be run by default. We will opt-in to running this test in CI w/ `cargo test -- --ignored`.
The `better_tls` unit test in the `better_tls.rs` integration test module was named before we added the `name_constraints` unit test. This commit renames the `better_tls` unit test to `path_building` to reflect that it runs the path building testsuite from bettertls.
We ignore the bettertls path building and name constraint test suites because they take longer than the rest of the webpki test suite, bogging down local development. In the context of CI we don't mind that extra runtime (on the order of ~15..30s) so this commit updates the CI configuration to always run ignored tests alongside the rest of the test suite.
This commit updates CI to use `cargo hack` to test the feature powerset, ensuring that we can catch feature interplay related breakages early. Unlike w/ the main Rustls repo the webpki powerset is small and the test completes in <30s, so it's fair to include in the default CI instead of needing to be separated into a separate daily tests workflow.
cpu
force-pushed
the
cpu-retain-quick-test-flow
branch
from
ed573de to
dc8f5d8
Compare
cpu
changed the title
cpu
changed the title
djc
approved these changes
Sep 13, 2023
|
Going to merge, but as always happy to tweak further. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently our unit test runtime has been bogging down. Three tests in particular were slow relative to the rest:
test_too_many_path_callsfromverify_cert.rs, and the path building and name constraint test suites from the bettertls based integration test. Our entire project test suite minus those three tests completes in <0.5s. Runningtest_too_many_path_callsalone takes ~3s, and the better tls suite was ~8s.This branch does a few things to remedy this:
test_too_many_path_callsrun faster by setting a lowerbuild_chain_callsbudget.cargo hackfeature powerset test to CI.This should help keep regular development fast while still maintaining good coverage.