Skip to content
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

Feat: Add errorMessageFormatter #468

Merged
merged 10 commits into from
Jun 22, 2024

Conversation

Foxhoundn
Copy link

@Foxhoundn Foxhoundn commented May 21, 2024

Closes storybookjs/storybook#25131

What I did

The errorMessageFormatter property defines a function that will pre-format the error messages before they get reported in the CLI:

// .storybook/test-runner.ts
import type { TestRunnerConfig } from '@storybook/test-runner';

const config: TestRunnerConfig = {
  errorMessageFormatter: (message) => {
    // manipulate the error message as you like
    return message;
  },
};
export default config;

Here's a small example of the formatting
image

If the formatter throws an error, the test-runner will notify the user
image

Checklist for Contributors

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

  • Add or update documentation reflecting your changes in this repository
  • Request documentation updates in the test-runner docs website

Checklist for Maintainers

  • Make sure this PR contains one of the labels below:

    Available labels
    • skip-release: Skip any releases, e.g., documentation only changes, CI config etc.
    • patch: Upgrade patch version (e.g. 0.0.x)
    • minor: Upgrade patch version (e.g. 0.x.0)
    • major: Upgrade patch version (e.g. x.0.0)

@Foxhoundn
Copy link
Author

Foxhoundn commented May 21, 2024

@yannbf I couldn't push to your branch so had to create a sep PR, I did not update docs (is that in the main SB repo?) and I did not remove the changes that should not be merged which you added, I can remove them once this PR is approved.

@yannbf yannbf changed the base branch from main to next June 13, 2024 15:40
@yannbf
Copy link
Member

yannbf commented Jun 13, 2024

Hey @Foxhoundn thanks for providing the code! I think we need to think a bit more about this since it should be a common function for errors, and right now it seems to be applied only if an exception is coming from the play function.

@kasperpeulen @shilman do you want to chime in regarding:

  • functionality/behavior
  • API
  • naming

I would say this function should be applicable to other error events too, and receive as second argument some metadata (whenever possible) that contains at least the storyId, so users have more flexibility to know how to deal with formatting their messages.

@Foxhoundn
Copy link
Author

Hey @Foxhoundn thanks for providing the code! I think we need to think a bit more about this since it should be a common function for errors, and right now it seems to be applied only if an exception is coming from the play function.

@kasperpeulen @shilman do you want to chime in regarding:

  • functionality/behavior
  • API
  • naming

I would say this function should be applicable to other error events too, and receive as second argument some metadata (whenever possible) that contains at least the storyId, so users have more flexibility to know how to deal with formatting their messages.

That would be splendid - I would love to have a list of stories that failed at the end of the test run too, right now it just says 5 test suites, 25 tests. Then you have to scroll through the whole history to find those 25 tests, but it would be wonderful to have the actual story list there, for easy orientation. Something to think about?

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be even better if we could fix the problem at its root and provide better error messages out of the box. But in the meantime, this change looks totally reasonable to me as a workaround. 💯

Copy link

codecov bot commented Jun 22, 2024

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 93.19%. Comparing base (bdd18e0) to head (1f25446).
Report is 35 commits behind head on next.

Current head 1f25446 differs from pull request most recent head 897ddba

Please upload reports for the commit 897ddba to get more accurate results.

Files Patch % Lines
.storybook/test-runner.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             next     #468      +/-   ##
==========================================
- Coverage   93.86%   93.19%   -0.67%     
==========================================
  Files          15       15              
  Lines         277      294      +17     
  Branches       71       78       +7     
==========================================
+ Hits          260      274      +14     
- Misses         17       20       +3     

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

@yannbf yannbf added feature request New feature or request minor Increment the minor version when merged labels Jun 22, 2024
@yannbf yannbf changed the title Format the full error message before passing it to the Error class Feat: Add errorMessageFormatter Jun 22, 2024
@yannbf yannbf merged commit 969e5dc into storybookjs:next Jun 22, 2024
4 of 5 checks passed
@yannbf yannbf mentioned this pull request Jun 22, 2024
Copy link

🚀 PR was released in v0.19.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request minor Increment the minor version when merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants