Add non-negativity sanity integration test - #112
Open
luisvlz0 wants to merge 3 commits into
Open
Conversation
Signed-off-by: Luis Valdez <[email protected]>
Signed-off-by: Luis Valdez <[email protected]>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new integration “sanity” suite to validate broad numeric invariants in /allocation query responses (finite numeric fields, non-negative costs/usage metrics, runtime minutes within window, and computed CPU/RAM/GPU efficiency bounds).
Changes:
- Introduces a new Go integration test that iterates across multiple allocation query windows/aggregations and validates non-negativity + finiteness constraints.
- Adds computed efficiency sanity checks derived from request-average and usage metrics.
- Adds a new Bats entry to run the sanity test package.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| test/integration/query/sanity/test.bats | Adds a Bats entry to execute the new allocation sanity Go test. |
| test/integration/query/sanity/non_negativity_sanity_test.go | Implements allocation response sanity validations (non-negative/finite fields, minutes-within-window, computed efficiency bounds). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+170
to
+173
| // checkComputedCPUEfficiency computes CPU efficiency from request-average and usage data | ||
| // and validates it is within [0, 1 + tolerance]. | ||
| // cpuEfficiency = cpuCoreHours / (cpuCoreRequestAverage * minutes / 60) | ||
| // Skips check if CPU request is negligible (< 1e-6). |
Comment on lines
+198
to
+201
| // checkComputedRAMEfficiency computes RAM efficiency from request-average and usage data | ||
| // and validates it is within [0, 1 + tolerance]. | ||
| // ramEfficiency = ramByteHours / (ramByteRequestAverage * minutes / 60) | ||
| // Skips check if RAM request is negligible (< 1e-6). |
Comment on lines
+226
to
+229
| // checkComputedGPUEfficiency computes GPU efficiency from request-average and usage data | ||
| // and validates it is within [0, 1 + tolerance]. | ||
| // gpuEfficiency = gpuHours / (gpuRequestAverage * minutes / 60) | ||
| // Skips check if GPU request is negligible (< 1e-6). |
Comment on lines
+22
to
+26
| // Efficiency tolerance allows for differences in time units and averaging methods. | ||
| // Request-average fields may use different bases (per-minute, per-second, etc.), | ||
| // so we allow efficiencies up to 5x to catch real issues while avoiding false | ||
| // positives from unit/reporting differences. | ||
| efficiencyTolerance = 5.0 |
…[email protected]> Co-authored-by: Copilot Autofix powered by AI <[email protected]> Signed-off-by: luisvlz0 <[email protected]>
Comment on lines
+20
to
+24
| // Efficiency tolerance allows for differences in time units and averaging methods. | ||
| // Request-average fields may use different bases (per-minute, per-second, etc.), | ||
| // so we allow efficiencies up to 5x to catch real issues while avoiding false | ||
| // positives from unit/reporting differences. | ||
| efficiencyTolerance = 5.0 |
| } | ||
|
|
||
| @test "query allocation non-negativity sanity checks" { | ||
| go test ./test/integration/query/sanity -run TestAllocationNonNegativitySanity -count=1 -v |
| return | ||
| } | ||
|
|
||
| cpuRequestedHours := item.CPUCoreRequestAverage * windowMinutes / 60.0 |
Collaborator
|
We actually had an option to test for negative costs on all fields but chose to do it only for idle costs. I feel the spirit of an integration test should be to test different parts of the system in conjuction, like prometheus providing inputs to opencost and the test comparing the /allocation values. There is a test for "negative idle costs" which does something similar, but checking for idle_costs is relevant because it is a computed by subtracting I feel this test is better suited to be a unit test rather than an integration test. @ameijer , what do you think? |
This file contains hidden or 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
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.
Adds allocation non-negativity sanity coverage for query responses.
This checks broad numeric invariants for allocation output, including finite numeric values, non-negative costs and usage metrics, runtime minutes within the returned window, and computed CPU/RAM efficiency sanity bounds.
The follow-up adjustment relaxes totalEfficiency validation so the test only requires it to be finite and non-negative, since observed demo values can exceed 1.0.