impl accumulation tests#70
Conversation
There was a problem hiding this comment.
Pull request overview
Adds integration coverage for OpenCost /allocation/summary accumulation behaviors and extends the Go API client request builder to support additional query parameters needed by those tests.
Changes:
- Added a new integration test file covering
accumulate,accumulateBy, andstepparameter behaviors for allocation summaries. - Extended
pkg/api.AllocationRequestto includeAccumulateByandStep, and to emit them in the query string.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| test/integration/api/allocation/accumulate_parameter_test.go | Introduces new integration tests validating accumulation modes, precedence, and invalid-parameter handling. |
| pkg/api/allocation.go | Adds AccumulateBy and Step fields to AllocationRequest and includes them in QueryString(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestAccumulateInvalidParamsReturnBadRequest(t *testing.T) { | ||
| t.Run("invalid accumulateBy", func(t *testing.T) { | ||
| resp := mustSummary(t, api.AllocationRequest{ | ||
| Window: "14d", | ||
| Aggregate: "namespace", | ||
| AccumulateBy: "not-valid", | ||
| }) | ||
| if resp.Code == 200 { | ||
| t.Fatalf("expected non-200 for invalid accumulateBy") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("invalid step", func(t *testing.T) { | ||
| resp := mustSummary(t, api.AllocationRequest{ | ||
| Window: "14d", | ||
| Aggregate: "namespace", | ||
| Accumulate: "week", | ||
| Step: "not-a-duration", | ||
| }) | ||
| if resp.Code == 200 { | ||
| t.Fatalf("expected non-200 for invalid step") | ||
| } |
There was a problem hiding this comment.
TestAccumulateInvalidParamsReturnBadRequest only asserts "non-200". Since the test name is specifically about BadRequest, it should assert the expected status/code (typically 400) so the test can’t pass if the API returns an unexpected error (e.g., 500) or an unexpected success payload with a non-200 Code field.
| func TestAccumulateLegacyTruthyValues(t *testing.T) { | ||
| tests := []string{"true", "1", "t", "TRUE"} | ||
|
|
||
| for _, value := range tests { | ||
| t.Run(value, func(t *testing.T) { | ||
| resp := mustSummary(t, api.AllocationRequest{ | ||
| Window: "14d", | ||
| Aggregate: "namespace", | ||
| Accumulate: value, | ||
| }) | ||
| if resp.Code != 200 { | ||
| t.Fatalf("expected 200, got %d", resp.Code) | ||
| } | ||
| if len(resp.Data.Sets) != 1 { | ||
| t.Fatalf("expected one accumulated set for accumulate=%q, got %d", value, len(resp.Data.Sets)) | ||
| } | ||
| requireNonEmptyAllocationsInEachSet(t, resp.Data.Sets) | ||
| }) | ||
| } |
There was a problem hiding this comment.
This new Go test file won’t run in CI via bats unless it’s added to test/integration/api/allocation/test.bats (that file currently enumerates each go test ..._test.go). Please wire this test into the bats suite (or adjust the harness to run go test ./test/integration/api/allocation/...) so the new coverage is actually executed.
| name: "quarter over 2 quarters", | ||
| windowStart: monthEnd.AddDate(0, -6, 0), | ||
| windowEnd: monthEnd, | ||
| accumulate: "quarter", | ||
| expectedCount: 2, | ||
| }, |
There was a problem hiding this comment.
The "quarter over 2 quarters" case isn’t aligned to calendar quarter boundaries: monthEnd.AddDate(0, -6, 0) will start mid‑quarter for most months, which can legitimately produce 3 quarter buckets (e.g., Nov→May spans Q4+Q1+Q2). To make the expected set count deterministic, compute the start of the current quarter (Jan/Apr/Jul/Oct 1st at 00:00 UTC) and use that as the window end, with the start set to exactly 2 quarters earlier.
| if len(resp.Data.Sets) != tc.expectedCount { | ||
| t.Fatalf("expected %d sets, got %d (window=%s, accumulate=%s)", tc.expectedCount, len(resp.Data.Sets), window, tc.accumulate) | ||
| } | ||
| requireNonEmptyAllocationsInEachSet(t, resp.Data.Sets) | ||
| }) |
There was a problem hiding this comment.
This test enforces requireNonEmptyAllocationsInEachSet for month/quarter buckets, which will fail in environments without 2+ months (and especially 6+ months) of retained allocation data. Other allocation integration tests in this repo stick to short windows like today/yesterday/week/14d, so consider either (a) relaxing the non-empty requirement for older buckets, or (b) skipping the month/quarter subtests when the response indicates no data in the requested window.
implements a test for the accumulate parameter in opencost