Skip to content

Commit

Permalink
Prevent infinite loop when error is thrown during play invocation
Browse files Browse the repository at this point in the history
In case the play function throws an error, we didn't properly
disambiguate it from the internal error thrown by
`forceHappoScreenshot`. This caused the test suite to re-render the last
step over and over (until eventually the browser crashed). To fix this,
I'm adding a `done` boolean to the step created by forceHappoScreenshot.
Each step is only allowed to be processed once.

It's a little annoying that the `errored` event details do not contain
any information about the thrown error. If it did, we wouldn't have to
do this complicated detection to see if the error is ours or someone
elses.
  • Loading branch information
trotzig committed Mar 19, 2024
1 parent 5d04598 commit 342b6c6
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 4 deletions.
19 changes: 19 additions & 0 deletions .storybook/Interactive.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,22 @@ export const Demo = {
});
},
};

export const InteractiveThrowsError = {
// This story exists to test what happens when the play function throws an
// error that isn't caused by `forceHappoScreenshot`.
play: async ({ args, canvasElement, step }) => {
const canvas = within(canvasElement);
await new Promise((r) => setTimeout(r, 200));

await step('clicked', async () => {
await userEvent.click(canvas.getByRole('button'));
await expect(canvas.getByText('I was clicked')).toBeInTheDocument();
await forceHappoScreenshot('clicked');
throw new Error('Whoops');

// We will never reach this line
await forceHappoScreenshot('clicked2');
});
},
};
15 changes: 11 additions & 4 deletions src/register.js
Original file line number Diff line number Diff line change
Expand Up @@ -165,9 +165,14 @@ function renderStory(story, { force = false } = {}) {
) {
channel.off('storyRenderPhaseChanged', handleRenderPhaseChanged);
clearTimeout(timeout);
const pausedAtStep =
forcedHappoScreenshotSteps[forcedHappoScreenshotSteps.length - 1];
if (pausedAtStep.done) {
// Already processed this step
return resolve();
}
return resolve({
pausedAtStep:
forcedHappoScreenshotSteps[forcedHappoScreenshotSteps.length - 1],
pausedAtStep,
});
}
if (ev.newPhase === 'playing') {
Expand Down Expand Up @@ -273,6 +278,8 @@ window.happo.nextExample = async () => {
} finally {
if (!pausedAtStep) {
currentIndex++;
} else {
pausedAtStep.done = true;
}
}
};
Expand All @@ -292,14 +299,14 @@ export function forceHappoScreenshot(stepLabel) {

if (
forcedHappoScreenshotSteps &&
forcedHappoScreenshotSteps.includes(stepLabel)
forcedHappoScreenshotSteps.some((s) => s.stepLabel === stepLabel)
) {
// ignore, this step has already been handled
return;
}

forcedHappoScreenshotSteps = forcedHappoScreenshotSteps || [];
forcedHappoScreenshotSteps.push(stepLabel);
forcedHappoScreenshotSteps.push({ stepLabel, done: false });

const e = new Error(`Forced screenshot with label "${stepLabel}"`);
e.type = 'ForcedHappoScreenshot';
Expand Down

0 comments on commit 342b6c6

Please sign in to comment.