Repository navigation
fix(runs): do not apply default 7d period when explicit runIds are supplied (#5110) #5112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,114 @@ | ||||||||||
| import { describe, expect, it, vi } from "vitest"; | ||||||||||
|
|
||||||||||
| // Prevent run-store / db / env singletons from constructing at import | ||||||||||
| vi.mock("~/db.server", () => ({})); | ||||||||||
| vi.mock("~/v3/runStore.server", () => ({ runStore: {} })); | ||||||||||
| vi.mock("~/env.server", () => ({ env: {} })); | ||||||||||
|
Comment on lines
+4
to
+6
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 New run tests replace real dependencies When these tests run, Learn moreThe repository's testing guidance requires tests without mocks, stubs, or fakes. These module replacements prevent import-time singletons from constructing, while the suite also passes empty fake Prisma and store objects. The converter does not use those objects for the tested inputs, but the test still relies on forbidden substitutes instead of a testable production dependency boundary. Example: The explicit-ID test succeeds with Recommended fix: Extract the pure time-filter conversion behind a dependency-free module and test it directly without Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||
|
|
||||||||||
| import { convertRunListInputOptionsToFilterRunsOptions } from "~/services/runsRepository/runsRepository.server"; | ||||||||||
|
|
||||||||||
| describe("convertRunListInputOptionsToFilterRunsOptions with explicit runIds vs filters (#5110)", () => { | ||||||||||
| const mockPrisma = {} as any; | ||||||||||
| const mockStore = {} as any; | ||||||||||
|
|
||||||||||
| it("leaves period undefined when explicit runIds are supplied without time filters", async () => { | ||||||||||
| const result = await convertRunListInputOptionsToFilterRunsOptions( | ||||||||||
| { | ||||||||||
| organizationId: "org_123", | ||||||||||
| projectId: "proj_123", | ||||||||||
| environmentId: "env_123", | ||||||||||
| runId: ["run_recent", "run_older_than_7_days"], | ||||||||||
| }, | ||||||||||
| mockPrisma, | ||||||||||
| mockStore | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| // Period must remain undefined so runs older than 7 days are not silently excluded (#5110) | ||||||||||
| expect(result.period).toBeUndefined(); | ||||||||||
|
Comment on lines
+26
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Regression test omits required bug label When the new unit suite checks
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||
| expect(result.runId).toEqual(["run_recent", "run_older_than_7_days"]); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("applies the default 7d period when no explicit runIds are supplied (filter-only)", async () => { | ||||||||||
| const result = await convertRunListInputOptionsToFilterRunsOptions( | ||||||||||
| { | ||||||||||
| organizationId: "org_123", | ||||||||||
| projectId: "proj_123", | ||||||||||
| environmentId: "env_123", | ||||||||||
| statuses: ["COMPLETED_SUCCESSFULLY"], | ||||||||||
| }, | ||||||||||
| mockPrisma, | ||||||||||
| mockStore | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| expect(result.period).toBe(7 * 24 * 60 * 60 * 1000); // 604800000 ms | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("applies the default 7d period when runId is an empty array", async () => { | ||||||||||
| const result = await convertRunListInputOptionsToFilterRunsOptions( | ||||||||||
| { | ||||||||||
| organizationId: "org_123", | ||||||||||
| projectId: "proj_123", | ||||||||||
| environmentId: "env_123", | ||||||||||
| runId: [], | ||||||||||
| }, | ||||||||||
| mockPrisma, | ||||||||||
| mockStore | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| expect(result.period).toBe(7 * 24 * 60 * 60 * 1000); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("honors explicit period when supplied alongside runIds", async () => { | ||||||||||
| const result = await convertRunListInputOptionsToFilterRunsOptions( | ||||||||||
| { | ||||||||||
| organizationId: "org_123", | ||||||||||
| projectId: "proj_123", | ||||||||||
| environmentId: "env_123", | ||||||||||
| runId: ["run_1"], | ||||||||||
| period: "30d", | ||||||||||
| }, | ||||||||||
| mockPrisma, | ||||||||||
| mockStore | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| expect(result.period).toBe(30 * 24 * 60 * 60 * 1000); | ||||||||||
| expect(result.runId).toEqual(["run_1"]); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("honors explicit from/to when supplied alongside runIds without defaulting period", async () => { | ||||||||||
| const from = Date.now() - 30 * 24 * 60 * 60 * 1000; | ||||||||||
| const to = Date.now(); | ||||||||||
| const result = await convertRunListInputOptionsToFilterRunsOptions( | ||||||||||
| { | ||||||||||
| organizationId: "org_123", | ||||||||||
| projectId: "proj_123", | ||||||||||
| environmentId: "env_123", | ||||||||||
| runId: ["run_1"], | ||||||||||
| from, | ||||||||||
| to, | ||||||||||
| }, | ||||||||||
| mockPrisma, | ||||||||||
| mockStore | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| expect(result.period).toBeUndefined(); | ||||||||||
| expect(result.from).toBe(from); | ||||||||||
| expect(result.to).toBe(to); | ||||||||||
| expect(result.runId).toEqual(["run_1"]); | ||||||||||
| }); | ||||||||||
|
|
||||||||||
| it("honors custom period on filter-only queries", async () => { | ||||||||||
| const result = await convertRunListInputOptionsToFilterRunsOptions( | ||||||||||
| { | ||||||||||
| organizationId: "org_123", | ||||||||||
| projectId: "proj_123", | ||||||||||
| environmentId: "env_123", | ||||||||||
| period: "14d", | ||||||||||
| }, | ||||||||||
| mockPrisma, | ||||||||||
| mockStore | ||||||||||
| ); | ||||||||||
|
|
||||||||||
| expect(result.period).toBe(14 * 24 * 60 * 60 * 1000); | ||||||||||
| }); | ||||||||||
| }); | ||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Bulk action fix lacks release note
When
runIdselects an older run, the new conversion includes it in bulk actions. This user-visible server fix has no required release note, so customers receive no announcement.Learn more
Repository guidance requires a
.server-changes/entry for user-visible fixes made only in server components. This change affects which runs bulk actions and run lists can include, and the PR changes only the webapp and its tests. Without an entry, the release summary omits the fix.Example: A customer whose bulk cancellation previously skipped an eight-day-old run will not see that selected older runs now work in the server release notes.
Recommended fix: Add a
.server-changes/markdown file witharea: webapp,type: fix, and one user-facing sentence describing the inclusion of older explicitly selected runs.Was this helpful? React with 👍 or 👎 to provide feedback.