fix: Fixed sync issue

[bryanjtc]

Fixes: #62
How to test: Modify Button.stories.js Demo story like this

export const Demo = (args) => (
  <button type="button" onClick={() => { throw new Error('boom') }}>
    Click
  </button>
);

Summary

This pull request addresses a problem with the test runner for Storybook where errors in one test (specifically the "demo" story in this case) were leaking and affecting subsequent tests (the "findme" story).

The root cause of this issue was identified as improper error handling in the transformPlaywright.ts file. To mitigate this, we have encapsulated the test function in a try-catch block to handle potential errors, and introduced a mechanism to ensure page errors are properly deregistered after each test execution.

Here's a brief summary of the changes:

• Encapsulated page.evaluate call within a try-catch block to handle errors.
• Defined pageErrorListener as a separate function so that it can be used to deregister from page errors after each test.
• Used page.off('pageerror', pageErrorListener); to deregister from page errors after each test.
• Added console logging for the error that occurs during page.evaluate.
• Updated related jest tests.

📦 Published PR as canary version: 0.13.1--canary.264.c65968e.0

✨ Test out this PR locally via:

npm install @storybook/[email protected]
# or 
yarn add @storybook/[email protected]

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (9db74b9) 76.66% compared to head (f55228c) 76.66%.

❗ Current head f55228c differs from pull request most recent head c65968e. Consider uploading reports for the commit c65968e to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             next     #264   +/-   ##
=======================================
  Coverage   76.66%   76.66%           
=======================================
  Files          11       11           
  Lines         180      180           
  Branches       40       40           
=======================================
  Hits          138      138           
  Misses         42       42           

Files Changed	Coverage Δ
src/playwright/transformPlaywright.ts	100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yannbf]

This is super awesome @bryanjtc, thanks for finding an alternative that does not affect performance!!! 🙌

Unfortunately I think there's still something going on. Let me show a few use cases where I noticed issues:

1. Error thrown on click:

export const Demo = (args) => (
  <button type="button" onClick={() => { throw new Error('boom') }}>
    Click
  </button>
);

Even though there is only one failure, it's logged as if there were many failures:

2. Failing play function:

export const Demo = (args) => (
  <button type="button" onClick={() => args.onSubmit('clicked')}>
    Click
  </button>
);

Demo.play = async ({ args, canvasElement }) => {
  await userEvent.click(within(canvasElement).getByRole('alert')); // this will fail, it's supposed to be button
  await expect(args.onSubmit).toHaveBeenCalledWith(expect.stringMatching(/([A-Z])\w+/gi));
};

Even though the play function is wrong and fails:

The test-runner detects it as success, just logging the failure as a console.error message instead:

Do you have any idea what's going on?

[bryanjtc]

This is super awesome @bryanjtc, thanks for finding an alternative that does not affect performance!!! 🙌

Unfortunately I think there's still something going on. Let me show a few use cases where I noticed issues:

1. Error thrown on click:

export const Demo = (args) => (
  <button type="button" onClick={() => { throw new Error('boom') }}>
    Click
  </button>
);

Even though there is only one failure, it's logged as if there were many failures:

**2. Failing play function:**

export const Demo = (args) => (
  <button type="button" onClick={() => args.onSubmit('clicked')}>
    Click
  </button>
);

Demo.play = async ({ args, canvasElement }) => {
  await userEvent.click(within(canvasElement).getByRole('alert')); // this will fail, it's supposed to be button
  await expect(args.onSubmit).toHaveBeenCalledWith(expect.stringMatching(/([A-Z])\w+/gi));
};

Even though the play function is wrong and fails:

The test-runner detects it as success, just logging the failure as a console.error message instead:

Do you have any idea what's going on?

Investigating

[yannbf]

Hey @bryanjtc did you manage to find anything new? Thanks!

[bryanjtc]

Hey @bryanjtc did you manage to find anything new? Thanks!

Due to my ongoing commitments, I haven't had a chance to review it yet. However, I anticipate having more availability in about a month. During that time, I'll make it a priority and provide a timely update.

[yannbf]

Hey @bryanjtc did you manage to find anything new? Thanks!

Due to my ongoing commitments, I haven't had a chance to review it yet. However, I anticipate having more availability in about a month. During that time, I'll make it a priority and provide a timely update.

You're the best! <3

[bryanjtc]

Errors identified in the modified Component Demo for case 1: Error thrown on click

1. The story fails due to the failure of the expect function.
2. The button triggers an error ('boom') when onClick is invoked during the execution of the story.
3. The pageErrorListener is invoked as a result of the 'boom' error being thrown within the story.

Errors 1 and 3 are expected. Looking into catching error 2.

[bryanjtc]

For now, I think we should show the 3 errors, based on this discussion. https://stackoverflow.com/a/74350185.
I tested case 2, Failing play function and I only have one error.

[yannbf]

If we're just catching and throwing the error, does it make sense to wrap it in a try catch? in this case, result = null will never be called because the error is thrown before that line

[yannbf]

Errors identified in the modified Component Demo for case 1: Error thrown on click

1. The story fails due to the failure of the expect function.
2. The button triggers an error ('boom') when onClick is invoked during the execution of the story.
3. The pageErrorListener is invoked as a result of the 'boom' error being thrown within the story.

Errors 1 and 3 are expected. Looking into catching error 2.

Thank you for researching! I think there are a few things going on, and I added a commit to help us identify them, which will add some logs to the CLI.

I made only two stories be loaded in Button.stories, and I run yarn test-storybook button for a focused run. We should undo that commit later of course.

When the Demo story comes before Primary, the tests fail, leak and somehow duplicate:

When the Primary story comes before Demo, the tests abort as the browser was closed:

When looking at the logs, I realized that storyRendered (which makes a tests pass) is emitted even though playFunctionThrewException was emitted. That's one potential issue, as once any error happens, nothing else should be emitted. I'm very confused about the order of stories impacting the browser being closed or not. Hopefully this will help us identify what really is going on!

[yannbf]

This PR was superceeded by #404