Test improvments#276
Conversation
Reviewer's GuideAdds an environment-driven test timeout scaling utility and wires it through long-running integration tests, plus introduces dedicated Make targets to run attestation and trusted_execution_cluster tests separately and documents their usage. Flow diagram for new test Make targets and timeout multiplierflowchart LR
dev[Developer] --> attestation_tests[make attestation-tests]
dev[Developer] --> trusted_cluster_tests[make trusted_execution_cluster-tests]
attestation_tests --> cargo_attestation[cargo test --test attestation --features virtualization]
trusted_cluster_tests --> cargo_trusted[cargo test --test trusted_execution_cluster --features virtualization]
env_timeout[TEST_TIMEOUT_MULTIPLIER env var] --> timeout_util[Test timeout scaling utility]
cargo_attestation --> timeout_util
cargo_trusted --> timeout_util
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
timeout_multiplier()helper currently allowsTEST_TIMEOUT_MULTIPLIERvalues of 0 or negative (which can lead to zero or effectively unbounded timeouts via float→u64 casting); consider clamping to a sane positive range (e.g.>= 1.0and some upper bound) before applying it. - The new
attestation-testsandtrusted_execution_cluster-testsMakefile targets duplicate most of theintegration-testscommand; consider factoring out the commonRUST_LOG/image/env setup into a single variable or pattern rule to reduce repetition and keep them in sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `timeout_multiplier()` helper currently allows `TEST_TIMEOUT_MULTIPLIER` values of 0 or negative (which can lead to zero or effectively unbounded timeouts via float→u64 casting); consider clamping to a sane positive range (e.g. `>= 1.0` and some upper bound) before applying it.
- The new `attestation-tests` and `trusted_execution_cluster-tests` Makefile targets duplicate most of the `integration-tests` command; consider factoring out the common `RUST_LOG`/image/env setup into a single variable or pattern rule to reduce repetition and keep them in sync.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
504c2c9 to
7782c98
Compare
Jakob-Naucke
left a comment
There was a problem hiding this comment.
commit message nit: separately, wrap lines
7782c98 to
a21f7eb
Compare
d08c496 to
7999ac3
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Jakob-Naucke, yairpod The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Looks good to me, thanks |
Add separate targets for attestation and trusted_execution_cluster tests so they can be run seperatly. Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: Opus:4.6
Add $TEST_TIMEOUT_MULTIPLIER env variable to scale all test timeouts (e.g. 2, 1.5) for systems where VM and resource operations run slower. Signed-off-by: Yair Podemsky <ypodemsk@redhat.com> Assisted-by: Opus:4.6
7999ac3 to
12cdf53
Compare
|
New changes are detected. LGTM label has been removed. |
For easier testing I am adding make targets to run the attestation and trusted_execution_cluster seperatly, and an env variable TEST_TIMEOUT_MULTIPLIER that will increase the timeouts of all tests for slower systems.
Summary by Sourcery
Add configurable test timeout scaling and new make targets for running specific integration test suites.
New Features:
Enhancements: