From 187de2b26492bb866f2c6eca81f274aee99c94c1 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 12 Oct 2023 16:46:27 +1100 Subject: [PATCH 1/3] Fix issue with switching build without changing story --- .../VisualTests/VisualTests.stories.tsx | 53 +++---- src/utils/updateSelectedBuildInfo.test.ts | 147 ++++++++---------- src/utils/updateSelectedBuildInfo.ts | 3 + 3 files changed, 92 insertions(+), 111 deletions(-) diff --git a/src/screens/VisualTests/VisualTests.stories.tsx b/src/screens/VisualTests/VisualTests.stories.tsx index 2b0657e4..d23f79c5 100644 --- a/src/screens/VisualTests/VisualTests.stories.tsx +++ b/src/screens/VisualTests/VisualTests.stories.tsx @@ -573,30 +573,29 @@ export const PendingLocalBuildCapturedStory = { "https://www.figma.com/file/GFEbCgCVDtbZhngULbw2gP/Visual-testing-in-Storybook?type=design&node-id=2303-374529&t=qjmuGHxoALrVuhvX-0" ), }, - // NOTE: this does not current work, the story is auto-selected - // play: async ({ canvasElement, args, argsByTarget }) => { - // // We have to wait a moment for the story to render - // const canvas = within(canvasElement); - // await canvas.findAllByText("Latest"); - - // // Ensure we don't switch to the new build, the user has to opt-in - // mock(args.setSelectedBuildInfo!).mock.calls.forEach(([updater]) => { - // const result = typeof updater === "function" ? updater(args.selectedBuildInfo) : updater; - // expect(result).toEqual(args.selectedBuildInfo); // Unchanged - // }); - - // // Now opt in - // const viewLatestSnapshot = (await canvas.findAllByText("View latest snapshot"))[0]; - // await userEvent.click(viewLatestSnapshot); - - // const graphqlArgs = argsByTarget.graphql?.$graphql as typeof args.$graphql; // We need to type argsByTarget - // await waitFor(() => { - // expect(args.setSelectedBuildInfo).toHaveBeenCalledWith({ - // buildId: graphqlArgs?.AddonVisualTestsBuild?.lastBuildOnBranch?.id, - // storyId: meta.args.storyId, - // }); - // }); - // }, + play: async ({ canvasElement, args, argsByTarget }) => { + // We have to wait a moment for the story to render + const canvas = within(canvasElement); + await canvas.findAllByText("Latest"); + + // Ensure we don't switch to the new build, the user has to opt-in + mock(args.setSelectedBuildInfo!).mock.calls.forEach(([updater]) => { + const result = typeof updater === "function" ? updater(args.selectedBuildInfo) : updater; + expect(result).toEqual(args.selectedBuildInfo); // Unchanged + }); + + // Now opt in + const viewLatestSnapshot = (await canvas.findAllByText("View latest snapshot"))[0]; + await userEvent.click(viewLatestSnapshot); + + const graphqlArgs = argsByTarget.graphql?.$graphql as typeof args.$graphql; // We need to type argsByTarget + await waitFor(() => { + expect(args.setSelectedBuildInfo).toHaveBeenCalledWith({ + buildId: graphqlArgs?.AddonVisualTestsBuild?.lastBuildOnBranch?.id, + storyId: meta.args.storyId, + }); + }); + }, } satisfies Story; /** @@ -622,9 +621,8 @@ export const PendingCIBuildCapturedStory = { "https://www.figma.com/file/GFEbCgCVDtbZhngULbw2gP/Visual-testing-in-Storybook?type=design&node-id=2303-374529&t=qjmuGHxoALrVuhvX-0" ), }, - // NOTE: this does not current work, the story is auto-selected // Should behave the same as when the local build captures the current story - // play: PendingLocalBuildCapturedStory.play, + play: PendingLocalBuildCapturedStory.play, } satisfies Story; export const Pending = { @@ -862,8 +860,7 @@ export const CIBuildNewer = { }, }, // Similarly to a captured story, we should only switch when the user is ready - // NOTE: this does not current work, the story is auto-selected - // play: PendingLocalBuildCapturedStory.play, + play: PendingLocalBuildCapturedStory.play, } satisfies Story; /** The new build is newer than the story build and the git info */ diff --git a/src/utils/updateSelectedBuildInfo.test.ts b/src/utils/updateSelectedBuildInfo.test.ts index 55fa86c6..5a965bdb 100644 --- a/src/utils/updateSelectedBuildInfo.test.ts +++ b/src/utils/updateSelectedBuildInfo.test.ts @@ -1,102 +1,83 @@ import { updateSelectedBuildInfo } from "./updateSelectedBuildInfo"; -it("does nothing if there is no next build", () => { - expect( - updateSelectedBuildInfo(undefined, { - shouldSwitchToLastBuildOnBranch: false, - lastBuildOnBranchId: undefined, - storyId: "storyId", - }) - ).toEqual(undefined); -}); - -it("sets the story build from the next build, simple", () => { - expect( - updateSelectedBuildInfo(undefined, { - shouldSwitchToLastBuildOnBranch: true, - lastBuildOnBranchId: "lastBuildOnBranchId", - storyId: "storyId", - }) - ).toEqual({ - buildId: "lastBuildOnBranchId", - storyId: "storyId", +describe("with no selected build", () => { + it("does nothing if there is no next build", () => { + expect( + updateSelectedBuildInfo(undefined, { + shouldSwitchToLastBuildOnBranch: false, + lastBuildOnBranchId: undefined, + storyId: "storyId", + }) + ).toEqual(undefined); }); -}); -// We should remain on the "new build" screen until we see a completed story -it("does not set the story build from the next build, if the next build should not be switched to", () => { - expect( - updateSelectedBuildInfo(undefined, { - shouldSwitchToLastBuildOnBranch: false, - lastBuildOnBranchId: "lastBuildOnBranchId", - storyId: "storyId", - }) - ).toEqual(undefined); -}); - -it("updates the story build from the next build, simple", () => { - expect( - updateSelectedBuildInfo( - { buildId: "oldBuildId", storyId: "storyId" }, - { + it("sets the story build from the next build, simple", () => { + expect( + updateSelectedBuildInfo(undefined, { shouldSwitchToLastBuildOnBranch: true, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "storyId", - } - ) - ).toEqual({ - buildId: "lastBuildOnBranchId", - storyId: "storyId", + }) + ).toEqual({ + buildId: "lastBuildOnBranchId", + storyId: "storyId", + }); }); -}); -it("does not update the story build from the next build, if the next build should not be switched to", () => { - expect( - updateSelectedBuildInfo( - { buildId: "oldBuildId", storyId: "storyId" }, - { + // We should remain on the "new build" screen until we see a completed story + it("does not set the story build from the next build, if the next build should not be switched to", () => { + expect( + updateSelectedBuildInfo(undefined, { shouldSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "storyId", - } - ) - ).toEqual({ buildId: "oldBuildId", storyId: "storyId" }); + }) + ).toEqual(undefined); + }); }); -it("updates the storyId, simple", () => { - expect( - updateSelectedBuildInfo( - { - buildId: "lastBuildOnBranchId", - storyId: "storyId", - }, - { - shouldSwitchToLastBuildOnBranch: true, - lastBuildOnBranchId: "lastBuildOnBranchId", - storyId: "newStoryId", - } - ) - ).toEqual({ - buildId: "lastBuildOnBranchId", - storyId: "newStoryId", +describe("with a selected build, when not changing story", () => { + it("does not update the story build from the next build, no matter what", () => { + expect( + updateSelectedBuildInfo( + { buildId: "oldBuildId", storyId: "storyId" }, + { + shouldSwitchToLastBuildOnBranch: true, + lastBuildOnBranchId: "lastBuildOnBranchId", + storyId: "storyId", + } + ) + ).toEqual({ buildId: "oldBuildId", storyId: "storyId" }); }); }); -it("updates the storyId, keeping the old build if the next build should not be switched to", () => { - expect( - updateSelectedBuildInfo( - { - buildId: "oldBuildId", - storyId: "storyId", - }, - { - shouldSwitchToLastBuildOnBranch: false, - lastBuildOnBranchId: "lastBuildOnBranchId", - storyId: "newStoryId", - } - ) - ).toEqual({ - buildId: "oldBuildId", - storyId: "newStoryId", +describe("with a selected build, when changing story", () => { + it("updates the story build from the next build, simple", () => { + expect( + updateSelectedBuildInfo( + { buildId: "oldBuildId", storyId: "storyId" }, + { + shouldSwitchToLastBuildOnBranch: true, + lastBuildOnBranchId: "lastBuildOnBranchId", + storyId: "newStoryId", + } + ) + ).toEqual({ + buildId: "lastBuildOnBranchId", + storyId: "newStoryId", + }); + }); + + it("does not update the story build from the next build, if the next build should not be switched to", () => { + expect( + updateSelectedBuildInfo( + { buildId: "oldBuildId", storyId: "storyId" }, + { + shouldSwitchToLastBuildOnBranch: false, + lastBuildOnBranchId: "lastBuildOnBranchId", + storyId: "newStoryId", + } + ) + ).toEqual({ buildId: "oldBuildId", storyId: "newStoryId" }); }); }); diff --git a/src/utils/updateSelectedBuildInfo.ts b/src/utils/updateSelectedBuildInfo.ts index 2d00b82e..fb7a0ad5 100644 --- a/src/utils/updateSelectedBuildInfo.ts +++ b/src/utils/updateSelectedBuildInfo.ts @@ -15,6 +15,9 @@ export function updateSelectedBuildInfo( storyId: string; } ) { + // Never touch the selected build if we don't change story + if (oldSelectedBuildInfo?.storyId === storyId) return oldSelectedBuildInfo; + if (!shouldSwitchToLastBuildOnBranch) { if (!oldSelectedBuildInfo) return undefined; From 54ea00ff3cd3c2c94dc1a24466a58030282bb41d Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 12 Oct 2023 16:50:17 +1100 Subject: [PATCH 2/3] Add a story testing the change story behaviour --- .../VisualTests/VisualTests.stories.tsx | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/screens/VisualTests/VisualTests.stories.tsx b/src/screens/VisualTests/VisualTests.stories.tsx index d23f79c5..272cea87 100644 --- a/src/screens/VisualTests/VisualTests.stories.tsx +++ b/src/screens/VisualTests/VisualTests.stories.tsx @@ -118,6 +118,7 @@ const meta = { }, args: { setSelectedBuildInfo: action("setSelectedBuildInfo"), + dismissBuildError: action("dismissBuildError"), gitInfo: { userEmailHash: "xyz987", branch: "feature-branch", @@ -625,6 +626,23 @@ export const PendingCIBuildCapturedStory = { play: PendingLocalBuildCapturedStory.play, } satisfies Story; +/** + * Now what happens when the user actually clicks to change story? + */ +export const PendingCIBuildCapturedStoryAndUserChangedStory = { + args: { + selectedBuildInfo: { buildId: pendingBuild.id, storyId: "old--story" }, + $graphql: PendingLocalBuildCapturedStory.args.$graphql, + }, + parameters: { + ...withFigmaDesign( + "https://www.figma.com/file/GFEbCgCVDtbZhngULbw2gP/Visual-testing-in-Storybook?type=design&node-id=2303-374529&t=qjmuGHxoALrVuhvX-0" + ), + }, + // In this case, we *should* switch to the new build + play: EmptyBranchLocalBuildCapturedCurrentStory.play, +} satisfies Story; + export const Pending = { args: { selectedBuildInfo: { buildId: pendingBuild.id, storyId: meta.args.storyId }, From 19df40ae4c5e0bc26c8dc4d911cf73b3daa22333 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 12 Oct 2023 17:09:04 +1100 Subject: [PATCH 3/3] Apply the same logic to new stories as we do to the first build In terms of replacing the selectedBuild with the lastBuildOnBranch and switching selectedBuilId --- .../VisualTests/VisualTests.stories.tsx | 4 --- src/screens/VisualTests/VisualTests.tsx | 33 +++++++++++++++---- src/utils/updateSelectedBuildInfo.test.ts | 32 ++++++++++++++---- src/utils/updateSelectedBuildInfo.ts | 6 +++- 4 files changed, 58 insertions(+), 17 deletions(-) diff --git a/src/screens/VisualTests/VisualTests.stories.tsx b/src/screens/VisualTests/VisualTests.stories.tsx index 272cea87..4195741b 100644 --- a/src/screens/VisualTests/VisualTests.stories.tsx +++ b/src/screens/VisualTests/VisualTests.stories.tsx @@ -373,10 +373,6 @@ export const StoryAddedInSelectedBuild = { }, } satisfies Story; -/** - * Although this state doesn't immediately render the captured story (it probably should), - * it should switch to the lastBuildOnBranch immediately. - */ export const StoryAddedInLastBuildOnBranchNotInSelected = { args: { selectedBuildInfo: { buildId: pendingBuild.id, storyId: meta.args.storyId }, diff --git a/src/screens/VisualTests/VisualTests.tsx b/src/screens/VisualTests/VisualTests.tsx index bf13c13a..129e7138 100644 --- a/src/screens/VisualTests/VisualTests.tsx +++ b/src/screens/VisualTests/VisualTests.tsx @@ -154,12 +154,22 @@ export const VisualTestsWithoutSelectedBuildId = ({ !!lastBuildOnBranch && lastBuildOnBranchStoryTests.every(({ status }) => status !== TestStatus.InProgress); + // We may end up using the lastBuildOnBranch as selected build if we know we are about + // to switch to it. + const selectedBuildFromData = getFragment(FragmentSelectedBuildFields, data?.selectedBuild); + + const selectedBuildHasStory = + selectedBuildFromData && + "testsForStory" in selectedBuildFromData && + selectedBuildFromData.testsForStory?.nodes?.length; + // Before we set the storyInfo, we use the lastBuildOnBranch for story data if it's ready - const selectedBuild = getFragment( - FragmentSelectedBuildFields, - data?.selectedBuild ?? - (lastBuildOnBranchCompletedStory ? data?.project?.lastBuildOnBranch : undefined) - ); + const selectedBuild = selectedBuildHasStory + ? selectedBuildFromData + : getFragment( + FragmentSelectedBuildFields, + lastBuildOnBranchCompletedStory ? data?.project?.lastBuildOnBranch : undefined + ); const selectedBuildHasCorrectBranch = selectedBuild?.branch === gitInfo.branch; // Currently only used by the sidebar button to show a blue dot ("build outdated") @@ -195,19 +205,30 @@ export const VisualTestsWithoutSelectedBuildId = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [JSON.stringify(buildStatusUpdate), updateBuildStatus]); + // Should means "we'd like you to but we'll let you decide" const shouldSwitchToLastBuildOnBranch = canSwitchToLastBuildOnBranch && lastBuildOnBranchCompletedStory; + // Force means "we are going to change you to this build" + const forceSwitchToLastBuildOnBranch = shouldSwitchToLastBuildOnBranch && !selectedBuildHasStory; + // Ensure we are holding the right story build useEffect(() => { setSelectedBuildInfo((oldSelectedBuildInfo) => updateSelectedBuildInfo(oldSelectedBuildInfo, { shouldSwitchToLastBuildOnBranch, + forceSwitchToLastBuildOnBranch, lastBuildOnBranchId: lastBuildOnBranch?.id, storyId, }) ); - }, [shouldSwitchToLastBuildOnBranch, lastBuildOnBranch?.id, storyId, setSelectedBuildInfo]); + }, [ + shouldSwitchToLastBuildOnBranch, + forceSwitchToLastBuildOnBranch, + lastBuildOnBranch?.id, + storyId, + setSelectedBuildInfo, + ]); const switchToLastBuildOnBranch = useCallback( () => diff --git a/src/utils/updateSelectedBuildInfo.test.ts b/src/utils/updateSelectedBuildInfo.test.ts index 5a965bdb..32fcaab8 100644 --- a/src/utils/updateSelectedBuildInfo.test.ts +++ b/src/utils/updateSelectedBuildInfo.test.ts @@ -1,20 +1,22 @@ import { updateSelectedBuildInfo } from "./updateSelectedBuildInfo"; describe("with no selected build", () => { - it("does nothing if there is no next build", () => { + it("does nothing if there is no last build on branch", () => { expect( updateSelectedBuildInfo(undefined, { shouldSwitchToLastBuildOnBranch: false, + forceSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: undefined, storyId: "storyId", }) ).toEqual(undefined); }); - it("sets the story build from the next build, simple", () => { + it("sets the selected build from the last build on branch, simple", () => { expect( updateSelectedBuildInfo(undefined, { shouldSwitchToLastBuildOnBranch: true, + forceSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "storyId", }) @@ -25,10 +27,11 @@ describe("with no selected build", () => { }); // We should remain on the "new build" screen until we see a completed story - it("does not set the story build from the next build, if the next build should not be switched to", () => { + it("does not set the selected build from the last build on branch, if the last build on branch should not be switched to", () => { expect( updateSelectedBuildInfo(undefined, { shouldSwitchToLastBuildOnBranch: false, + forceSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "storyId", }) @@ -37,27 +40,43 @@ describe("with no selected build", () => { }); describe("with a selected build, when not changing story", () => { - it("does not update the story build from the next build, no matter what", () => { + it("does not update the selected build from the last build on branch, even if we should", () => { expect( updateSelectedBuildInfo( { buildId: "oldBuildId", storyId: "storyId" }, { shouldSwitchToLastBuildOnBranch: true, + forceSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "storyId", } ) ).toEqual({ buildId: "oldBuildId", storyId: "storyId" }); }); + + it("does update the selected build from the last build on branch, if forced", () => { + expect( + updateSelectedBuildInfo( + { buildId: "oldBuildId", storyId: "storyId" }, + { + shouldSwitchToLastBuildOnBranch: true, + forceSwitchToLastBuildOnBranch: true, + lastBuildOnBranchId: "lastBuildOnBranchId", + storyId: "storyId", + } + ) + ).toEqual({ buildId: "lastBuildOnBranchId", storyId: "storyId" }); + }); }); describe("with a selected build, when changing story", () => { - it("updates the story build from the next build, simple", () => { + it("updates the selected build from the last build on branch, simple", () => { expect( updateSelectedBuildInfo( { buildId: "oldBuildId", storyId: "storyId" }, { shouldSwitchToLastBuildOnBranch: true, + forceSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "newStoryId", } @@ -68,12 +87,13 @@ describe("with a selected build, when changing story", () => { }); }); - it("does not update the story build from the next build, if the next build should not be switched to", () => { + it("does not update the selected build from the last build on branch, if the last build on branch should not be switched to", () => { expect( updateSelectedBuildInfo( { buildId: "oldBuildId", storyId: "storyId" }, { shouldSwitchToLastBuildOnBranch: false, + forceSwitchToLastBuildOnBranch: false, lastBuildOnBranchId: "lastBuildOnBranchId", storyId: "newStoryId", } diff --git a/src/utils/updateSelectedBuildInfo.ts b/src/utils/updateSelectedBuildInfo.ts index fb7a0ad5..ba9bafa6 100644 --- a/src/utils/updateSelectedBuildInfo.ts +++ b/src/utils/updateSelectedBuildInfo.ts @@ -7,16 +7,20 @@ export function updateSelectedBuildInfo( oldSelectedBuildInfo: SelectedBuildInfo | undefined, { shouldSwitchToLastBuildOnBranch, + forceSwitchToLastBuildOnBranch, lastBuildOnBranchId, storyId, }: { shouldSwitchToLastBuildOnBranch: boolean; + forceSwitchToLastBuildOnBranch: boolean; lastBuildOnBranchId?: string; storyId: string; } ) { // Never touch the selected build if we don't change story - if (oldSelectedBuildInfo?.storyId === storyId) return oldSelectedBuildInfo; + if (oldSelectedBuildInfo?.storyId === storyId && !forceSwitchToLastBuildOnBranch) { + return oldSelectedBuildInfo; + } if (!shouldSwitchToLastBuildOnBranch) { if (!oldSelectedBuildInfo) return undefined;