Repository navigation
fix(runs): do not apply default 7d period when explicit runIds are supplied (#5110) #5111
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. 🟡 Run-filter tests use forbidden mocks The new run-filter tests use Learn moreThe webapp tests replace database, run-store, and environment modules before loading the converter. The repository's testing instructions prohibit mocks, stubs, and fakes; these tests also pass placeholder objects as dependencies. Keeping those replacements means the tests do not exercise the prescribed production dependency setup. Example: The first case passes Recommended fix: Restructure the tests around real dependencies, using the repository's testcontainers helpers where database services are needed. If the converter needs isolation, extract pure time-filter conversion into a dependency-free helper and test that without mocking imports or dependencies. 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) | ||||||||
|
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. 🟡 Run-filter test lacks regression annotation The new unit test's
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||
| expect(result.period).toBeUndefined(); | ||||||||
| 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
convertRunListInputOptionsToFilterRunsOptionsincludes older selected runs, users see a server-only behavior fix without a release note. The release changelog omits this fix.Learn more
Explicit run-ID selections now include older runs in counts and bulk actions. This is a user-facing change in the webapp, with no package changes. The repository's server-change policy requires a
.server-changes/file for such fixes, but this PR adds none. Users reading the release notes will miss a change they may have encountered.Example: A user whose selected 30-day-old run was previously skipped can now cancel it through a bulk action, but the server release summary does not mention this correction.
Recommended fix: Add a
.server-changes/Markdown entry witharea: webapp,type: fix, and a one-line description of the user-visible behavior.Was this helpful? React with 👍 or 👎 to provide feedback.