From cfc53fb4d3f7ca6e0eb6338a4262f75ce4304b47 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 16:08:54 +0000 Subject: [PATCH 1/7] chore: Add benchmark logs to test --- .../TemplateSchedulePage.test.tsx | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index 818fb24e4c265..9dc2310c519db 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -1,3 +1,4 @@ +/*eslint eslint-comments/disable-enable-pair: error -- Remove after bottlenecks or causes of timeouts for testing have been figured out */ import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as API from "api/api"; @@ -36,21 +37,27 @@ const renderTemplateSchedulePage = async () => { await waitForLoaderToBeRemoved(); }; +// Extracts all properties from TemplateScheduleFormValues that have a key that +// ends in _ms, and makes those properties optional. Defined as mapped type to +// ensure this stays in sync as TemplateScheduleFormValues changes +type FillAndSubmitConfig = { + [Key in keyof TemplateScheduleFormValues as Key extends `${string}_ms` + ? Key + : never]?: TemplateScheduleFormValues[Key] | undefined; +}; + +/*eslint-disable no-console -- Start benchmarks */ const fillAndSubmitForm = async ({ default_ttl_ms, max_ttl_ms, failure_ttl_ms, time_til_dormant_ms, time_til_dormant_autodelete_ms, -}: { - default_ttl_ms?: number; - max_ttl_ms?: number; - failure_ttl_ms?: number; - time_til_dormant_ms?: number; - time_til_dormant_autodelete_ms?: number; -}) => { +}: FillAndSubmitConfig) => { + console.time("form - full function"); const user = userEvent.setup(); + console.time("form - ttl"); if (default_ttl_ms) { const defaultTtlField = await screen.findByLabelText( "Default autostop (hours)", @@ -58,27 +65,36 @@ const fillAndSubmitForm = async ({ await user.clear(defaultTtlField); await user.type(defaultTtlField, default_ttl_ms.toString()); } + console.timeEnd("form - ttl"); + console.time("form - max_ttl"); if (max_ttl_ms) { const maxTtlField = await screen.findByLabelText("Max lifetime (hours)"); + await user.clear(maxTtlField); await user.type(maxTtlField, max_ttl_ms.toString()); } + console.timeEnd("form - max_ttl"); + console.time("form - failure_ttl"); if (failure_ttl_ms) { const failureTtlField = screen.getByRole("checkbox", { name: /Failure Cleanup/i, }); await user.type(failureTtlField, failure_ttl_ms.toString()); } + console.timeEnd("form - failure_ttl"); + console.time("form - dormant"); if (time_til_dormant_ms) { const inactivityTtlField = screen.getByRole("checkbox", { name: /Dormancy Threshold/i, }); await user.type(inactivityTtlField, time_til_dormant_ms.toString()); } + console.timeEnd("form - dormant"); + console.time("form - auto-delete"); if (time_til_dormant_autodelete_ms) { const dormancyAutoDeletionField = screen.getByRole("checkbox", { name: /Dormancy Auto-Deletion/i, @@ -88,16 +104,24 @@ const fillAndSubmitForm = async ({ time_til_dormant_autodelete_ms.toString(), ); } + console.timeEnd("form - auto-delete"); + console.time("form - submit"); const submitButton = await screen.findByText( FooterFormLanguage.defaultSubmitLabel, ); await user.click(submitButton); + console.timeEnd("form - submit"); - // User needs to confirm dormancy and autodeletion fields. + console.time("form - confirm"); + // User needs to confirm dormancy and auto-deletion fields. const confirmButton = await screen.findByTestId("confirm-button"); await user.click(confirmButton); + console.timeEnd("form - confirm"); + + console.timeEnd("form - full function"); }; +/*eslint-enable no-console -- End benchmarks */ describe("TemplateSchedulePage", () => { beforeEach(() => { @@ -109,15 +133,16 @@ describe("TemplateSchedulePage", () => { jest.spyOn(API, "getExperiments").mockResolvedValue(["workspace_actions"]); }); - it("succeeds", async () => { + it("Calls the API when user fills in and submits a form", async () => { await renderTemplateSchedulePage(); jest.spyOn(API, "updateTemplateMeta").mockResolvedValueOnce({ ...MockTemplate, ...validFormValues, }); + await fillAndSubmitForm(validFormValues); await waitFor(() => expect(API.updateTemplateMeta).toBeCalledTimes(1)); - }); + }, 15_000); test("default and max ttl is converted to and from hours", async () => { await renderTemplateSchedulePage(); From 3191ddbc77da199b0ac78cba885d74365df3a593 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 17:02:35 +0000 Subject: [PATCH 2/7] chore: Remove benchmark logic --- .../TemplateSchedulePage.test.tsx | 22 +------------------ 1 file changed, 1 insertion(+), 21 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index 9dc2310c519db..efc98e6a90a2f 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -1,4 +1,3 @@ -/*eslint eslint-comments/disable-enable-pair: error -- Remove after bottlenecks or causes of timeouts for testing have been figured out */ import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as API from "api/api"; @@ -46,7 +45,6 @@ type FillAndSubmitConfig = { : never]?: TemplateScheduleFormValues[Key] | undefined; }; -/*eslint-disable no-console -- Start benchmarks */ const fillAndSubmitForm = async ({ default_ttl_ms, max_ttl_ms, @@ -54,10 +52,8 @@ const fillAndSubmitForm = async ({ time_til_dormant_ms, time_til_dormant_autodelete_ms, }: FillAndSubmitConfig) => { - console.time("form - full function"); const user = userEvent.setup(); - console.time("form - ttl"); if (default_ttl_ms) { const defaultTtlField = await screen.findByLabelText( "Default autostop (hours)", @@ -65,36 +61,28 @@ const fillAndSubmitForm = async ({ await user.clear(defaultTtlField); await user.type(defaultTtlField, default_ttl_ms.toString()); } - console.timeEnd("form - ttl"); - console.time("form - max_ttl"); if (max_ttl_ms) { const maxTtlField = await screen.findByLabelText("Max lifetime (hours)"); await user.clear(maxTtlField); await user.type(maxTtlField, max_ttl_ms.toString()); } - console.timeEnd("form - max_ttl"); - console.time("form - failure_ttl"); if (failure_ttl_ms) { const failureTtlField = screen.getByRole("checkbox", { name: /Failure Cleanup/i, }); await user.type(failureTtlField, failure_ttl_ms.toString()); } - console.timeEnd("form - failure_ttl"); - console.time("form - dormant"); if (time_til_dormant_ms) { const inactivityTtlField = screen.getByRole("checkbox", { name: /Dormancy Threshold/i, }); await user.type(inactivityTtlField, time_til_dormant_ms.toString()); } - console.timeEnd("form - dormant"); - console.time("form - auto-delete"); if (time_til_dormant_autodelete_ms) { const dormancyAutoDeletionField = screen.getByRole("checkbox", { name: /Dormancy Auto-Deletion/i, @@ -104,24 +92,16 @@ const fillAndSubmitForm = async ({ time_til_dormant_autodelete_ms.toString(), ); } - console.timeEnd("form - auto-delete"); - console.time("form - submit"); const submitButton = await screen.findByText( FooterFormLanguage.defaultSubmitLabel, ); await user.click(submitButton); - console.timeEnd("form - submit"); - console.time("form - confirm"); // User needs to confirm dormancy and auto-deletion fields. const confirmButton = await screen.findByTestId("confirm-button"); await user.click(confirmButton); - console.timeEnd("form - confirm"); - - console.timeEnd("form - full function"); }; -/*eslint-enable no-console -- End benchmarks */ describe("TemplateSchedulePage", () => { beforeEach(() => { @@ -142,7 +122,7 @@ describe("TemplateSchedulePage", () => { await fillAndSubmitForm(validFormValues); await waitFor(() => expect(API.updateTemplateMeta).toBeCalledTimes(1)); - }, 15_000); + }); test("default and max ttl is converted to and from hours", async () => { await renderTemplateSchedulePage(); From 56873b80d7014374f16733124ba04169f1cdf943 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 17:09:03 +0000 Subject: [PATCH 3/7] chore: add hard cutoff for waitFor calls --- .../TemplateSchedulePage.test.tsx | 37 ++++++++++++++----- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index efc98e6a90a2f..298f170705663 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -1,4 +1,4 @@ -import { screen, waitFor } from "@testing-library/react"; +import { screen, waitFor, type waitForOptions } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as API from "api/api"; import { Language as FooterFormLanguage } from "components/FormFooter/FormFooter"; @@ -103,6 +103,12 @@ const fillAndSubmitForm = async ({ await user.click(confirmButton); }; +const waitForConfig = { + // Test averages about 13 seconds to complete; adding an extra three to + // account for spikes before definitely failing a test + timeout: 16_000, +} as const satisfies waitForOptions; + describe("TemplateSchedulePage", () => { beforeEach(() => { jest @@ -121,7 +127,10 @@ describe("TemplateSchedulePage", () => { }); await fillAndSubmitForm(validFormValues); - await waitFor(() => expect(API.updateTemplateMeta).toBeCalledTimes(1)); + await waitFor( + () => expect(API.updateTemplateMeta).toBeCalledTimes(1), + waitForConfig, + ); }); test("default and max ttl is converted to and from hours", async () => { @@ -133,16 +142,20 @@ describe("TemplateSchedulePage", () => { }); await fillAndSubmitForm(validFormValues); - await waitFor(() => expect(API.updateTemplateMeta).toBeCalledTimes(1)); - await waitFor(() => + await waitFor( + () => expect(API.updateTemplateMeta).toBeCalledTimes(1), + waitForConfig, + ); + + await waitFor(() => { expect(API.updateTemplateMeta).toBeCalledWith( "test-template", expect.objectContaining({ default_ttl_ms: (validFormValues.default_ttl_ms || 0) * 3600000, max_ttl_ms: (validFormValues.max_ttl_ms || 0) * 3600000, }), - ), - ); + ); + }, waitForConfig); }); test("failure, dormancy, and dormancy auto-deletion converted to and from days", async () => { @@ -154,8 +167,12 @@ describe("TemplateSchedulePage", () => { }); await fillAndSubmitForm(validFormValues); - await waitFor(() => expect(API.updateTemplateMeta).toBeCalledTimes(1)); - await waitFor(() => + await waitFor( + () => expect(API.updateTemplateMeta).toBeCalledTimes(1), + waitForConfig, + ); + + await waitFor(() => { expect(API.updateTemplateMeta).toBeCalledWith( "test-template", expect.objectContaining({ @@ -165,8 +182,8 @@ describe("TemplateSchedulePage", () => { time_til_dormant_autodelete_ms: (validFormValues.time_til_dormant_autodelete_ms || 0) * 86400000, }), - ), - ); + ); + }, waitForConfig); }); it("allows a default ttl of 7 days", () => { From 959f5fcd081808e9522fef126db22a3223ca8d42 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 17:20:42 +0000 Subject: [PATCH 4/7] refactor: clean up waitFor cut-off logic --- .../TemplateSchedulePage.test.tsx | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index 298f170705663..a2e84e71e2604 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -1,4 +1,4 @@ -import { screen, waitFor, type waitForOptions } from "@testing-library/react"; +import { screen, waitFor } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import * as API from "api/api"; import { Language as FooterFormLanguage } from "components/FormFooter/FormFooter"; @@ -103,11 +103,14 @@ const fillAndSubmitForm = async ({ await user.click(confirmButton); }; -const waitForConfig = { - // Test averages about 13 seconds to complete; adding an extra three to - // account for spikes before definitely failing a test - timeout: 16_000, -} as const satisfies waitForOptions; +function waitForWithCutoff(callback: () => void | Promise) { + return waitFor(callback, { + // Test file averages about 13 seconds to complete; adding an extra three + // seconds to account for spikes before definitely failing a test. Still + // falls under global config of 20 seconds + timeout: 16_000, + }); +} describe("TemplateSchedulePage", () => { beforeEach(() => { @@ -127,9 +130,8 @@ describe("TemplateSchedulePage", () => { }); await fillAndSubmitForm(validFormValues); - await waitFor( - () => expect(API.updateTemplateMeta).toBeCalledTimes(1), - waitForConfig, + await waitForWithCutoff(() => + expect(API.updateTemplateMeta).toBeCalledTimes(1), ); }); @@ -142,12 +144,11 @@ describe("TemplateSchedulePage", () => { }); await fillAndSubmitForm(validFormValues); - await waitFor( - () => expect(API.updateTemplateMeta).toBeCalledTimes(1), - waitForConfig, + await waitForWithCutoff(() => + expect(API.updateTemplateMeta).toBeCalledTimes(1), ); - await waitFor(() => { + await waitForWithCutoff(() => { expect(API.updateTemplateMeta).toBeCalledWith( "test-template", expect.objectContaining({ @@ -155,7 +156,7 @@ describe("TemplateSchedulePage", () => { max_ttl_ms: (validFormValues.max_ttl_ms || 0) * 3600000, }), ); - }, waitForConfig); + }); }); test("failure, dormancy, and dormancy auto-deletion converted to and from days", async () => { @@ -167,12 +168,11 @@ describe("TemplateSchedulePage", () => { }); await fillAndSubmitForm(validFormValues); - await waitFor( - () => expect(API.updateTemplateMeta).toBeCalledTimes(1), - waitForConfig, + await waitForWithCutoff(() => + expect(API.updateTemplateMeta).toBeCalledTimes(1), ); - await waitFor(() => { + await waitForWithCutoff(() => { expect(API.updateTemplateMeta).toBeCalledWith( "test-template", expect.objectContaining({ @@ -183,7 +183,7 @@ describe("TemplateSchedulePage", () => { (validFormValues.time_til_dormant_autodelete_ms || 0) * 86400000, }), ); - }, waitForConfig); + }); }); it("allows a default ttl of 7 days", () => { From 62fe36c47b69471b973756722accc2fa798ebc3c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 19:24:03 +0000 Subject: [PATCH 5/7] chore: add assertion that submit button is not disabled --- .../TemplateSchedulePage.test.tsx | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index a2e84e71e2604..736482241ff7d 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -54,6 +54,15 @@ const fillAndSubmitForm = async ({ }: FillAndSubmitConfig) => { const user = userEvent.setup(); + const submitButton = screen.getByRole("button", { + name: FooterFormLanguage.defaultSubmitLabel, + }); + + // This should be removed once we start removing disabled states from the UI; + // not worried about this getting forgotten, because it will trigger an error + // at some point once that migration starts + expect(submitButton).toBeDisabled(); + if (default_ttl_ms) { const defaultTtlField = await screen.findByLabelText( "Default autostop (hours)", @@ -93,9 +102,7 @@ const fillAndSubmitForm = async ({ ); } - const submitButton = await screen.findByText( - FooterFormLanguage.defaultSubmitLabel, - ); + expect(submitButton).not.toBeDisabled(); await user.click(submitButton); // User needs to confirm dormancy and auto-deletion fields. From 507dc80d30a0930e6d71e1e3232e4314d17083cd Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 21:03:52 +0000 Subject: [PATCH 6/7] chore: Remove disabled check at the start of the test --- .../TemplateSchedulePage.test.tsx | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index 736482241ff7d..165b8abe52079 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -54,15 +54,6 @@ const fillAndSubmitForm = async ({ }: FillAndSubmitConfig) => { const user = userEvent.setup(); - const submitButton = screen.getByRole("button", { - name: FooterFormLanguage.defaultSubmitLabel, - }); - - // This should be removed once we start removing disabled states from the UI; - // not worried about this getting forgotten, because it will trigger an error - // at some point once that migration starts - expect(submitButton).toBeDisabled(); - if (default_ttl_ms) { const defaultTtlField = await screen.findByLabelText( "Default autostop (hours)", @@ -102,6 +93,10 @@ const fillAndSubmitForm = async ({ ); } + const submitButton = screen.getByRole("button", { + name: FooterFormLanguage.defaultSubmitLabel, + }); + expect(submitButton).not.toBeDisabled(); await user.click(submitButton); From c4fff901bb45328c4de02e7c7039bd7314e5076d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Thu, 21 Sep 2023 17:51:48 +0000 Subject: [PATCH 7/7] fix: extend cutoff for waitFor config --- .../TemplateSchedulePage.test.tsx | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx index 165b8abe52079..3763e71560d73 100644 --- a/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx +++ b/site/src/pages/TemplateSettingsPage/TemplateSchedulePage/TemplateSchedulePage.test.tsx @@ -105,12 +105,16 @@ const fillAndSubmitForm = async ({ await user.click(confirmButton); }; +// One problem with the waitFor function is that if no additional config options +// are passed in, it will hang indefinitely as it keeps retrying an assertion. +// Even if Jest runs out of time and kills the test, you won't get a good error +// message. Adding options to force test to give up before test timeout function waitForWithCutoff(callback: () => void | Promise) { return waitFor(callback, { - // Test file averages about 13 seconds to complete; adding an extra three - // seconds to account for spikes before definitely failing a test. Still - // falls under global config of 20 seconds - timeout: 16_000, + // Defined to end 500ms before global cut-off time of 20s. Wanted to define + // this in terms of an exported constant from jest.config, but since Jest + // is CJS-based, that would've involved weird CJS-ESM interop issues + timeout: 19_500, }); }