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

Addon Test: Support story name as test description #29147

Merged
merged 9 commits into from
Sep 23, 2024

Conversation

InfiniteXyy
Copy link
Contributor

@InfiniteXyy InfiniteXyy commented Sep 18, 2024

What I did

I added a new feature to the newly published vitest-test plugin:

Now the test description will use the explictly settled name of the story, and then fallback to the exportName.

The requirement comes from that developers might want to have spaces/brackets/etc... in the test description, which is not allowed in exportName.

At the same time, this can also keep consistent with the name display in storybook preview.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

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
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

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

    Available labels
    • bug: Internal changes that fixes incorrect behavior.
    • maintenance: User-facing maintenance tasks.
    • dependencies: Upgrading (sometimes downgrading) dependencies.
    • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
    • cleanup: Minor cleanup style change. Will not show up in release changelog.
    • documentation: Documentation only changes. Will not show up in release changelog.
    • feature request: Introducing a new feature.
    • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
    • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

name before after diff z %
createSize 0 B 0 B 0 B - -
generateSize 77.4 MB 77.4 MB 0 B 1.54 0%
initSize 162 MB 162 MB 409 B -0.21 0%
diffSize 85 MB 85 MB 409 B -0.39 0%
buildSize 7.57 MB 7.57 MB 0 B -0.58 0%
buildSbAddonsSize 1.66 MB 1.66 MB 0 B -0.58 0%
buildSbCommonSize 195 kB 195 kB 0 B - 0%
buildSbManagerSize 2.34 MB 2.34 MB 0 B - 0%
buildSbPreviewSize 352 kB 352 kB 0 B - 0%
buildStaticSize 0 B 0 B 0 B - -
buildPrebuildSize 4.55 MB 4.55 MB 0 B -0.58 0%
buildPreviewSize 3.02 MB 3.02 MB 0 B -0.58 0%
testBuildSize 0 B 0 B 0 B - -
testBuildSbAddonsSize 0 B 0 B 0 B - -
testBuildSbCommonSize 0 B 0 B 0 B - -
testBuildSbManagerSize 0 B 0 B 0 B - -
testBuildSbPreviewSize 0 B 0 B 0 B - -
testBuildStaticSize 0 B 0 B 0 B - -
testBuildPrebuildSize 0 B 0 B 0 B - -
testBuildPreviewSize 0 B 0 B 0 B - -
name before after diff z %
createTime 7.2s 10.4s 3.1s -0.46 30.6%
generateTime 21.5s 21.2s -309ms 0.8 -1.5%
initTime 16s 17s 1s 0.64 5.9%
buildTime 10.6s 12.9s 2.3s 2.26 🔺18.1%
testBuildTime 0ms 0ms 0ms - -
devPreviewResponsive 7.3s 6.3s -947ms -1.03 -14.9%
devManagerResponsive 5s 4.3s -739ms -0.53 -17%
devManagerHeaderVisible 709ms 723ms 14ms -0.86 1.9%
devManagerIndexVisible 740ms 754ms 14ms -0.9 1.9%
devStoryVisibleUncached 1.1s 1.1s 7ms -0.89 0.6%
devStoryVisible 738ms 753ms 15ms -0.91 2%
devAutodocsVisible 639ms 645ms 6ms -1.19 0.9%
devMDXVisible 606ms 630ms 24ms -1.31 3.8%
buildManagerHeaderVisible 644ms 694ms 50ms -1.25 🔺7.2%
buildManagerIndexVisible 649ms 701ms 52ms -1.32 🔺7.4%
buildStoryVisible 735ms 768ms 33ms -1.02 4.3%
buildAutodocsVisible 639ms 604ms -35ms -1.4 🔰-5.8%
buildMDXVisible 572ms 630ms 58ms -0.92 9.2%

Greptile Summary

This PR enhances the Vitest plugin for Storybook by allowing customizable test descriptions using the story's 'name' property, improving readability and consistency with the Storybook preview.

  • Modified code/core/src/csf-tools/vitest-plugin/transformer.ts to prioritize story 'name' over 'exportName' for test descriptions
  • Added new test case in code/core/src/csf-tools/vitest-plugin/transformer.test.ts to verify the 'name' property usage
  • Updated docs/writing-tests/vitest-plugin.mdx with instructions on customizing test descriptions using the 'name' property
  • This change allows for more descriptive test names, including spaces and special characters not permitted in export names

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

code/core/src/csf-tools/vitest-plugin/transformer.ts Outdated Show resolved Hide resolved
Comment on lines 218 to 229
for (const prop of storyDeclarator.init.properties) {
if (
// Filter out the "name" property and return its value
prop.type === 'ObjectProperty' &&
prop.key.type === 'Identifier' &&
prop.value.type === 'StringLiteral' &&
prop.key.name === 'name'
) {
return prop.value.value;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider using find instead of for loop for better readability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With find, the type will be hard to define

Copy link

nx-cloud bot commented Sep 18, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8de3b0e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Hey @InfiniteXyy thanks a lot for your contribution! This was something we were planning to do, so thanks for speeding that up! I'd love if you could change the implemention to make it simpler, given that the underlying parsing tool (CsfTools) already handles extracting the story name.

I believe you can delete most of the code and use something like this:

const exportName = parsed._stories[exportName].name || exportName;
return getTestStatementForStory({
          exportName,
          node,
});

Test cases:

CSF1/2

export const Story = () => {}
Story.storyName = 'custom name'

CSF3

export const Story = () => {
  name: 'custom name'
}

Let me know how it goes and if you need anything else!

Comment on lines 382 to 384
### How do I customized the test description

We are using the exportName of a story definition by default, but you can provide a `name` property for the story to customize the test description. This is useful when you want have places, brackets or other special characters in the test description.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a code snippet alongside this so users can understand it better?

export const Story = {
  storyName: 'custom, descriptive name'
};
Suggested change
### How do I customized the test description
We are using the exportName of a story definition by default, but you can provide a `name` property for the story to customize the test description. This is useful when you want have places, brackets or other special characters in the test description.
### How do I customize the test description?
By default, the export name of a story is mapped to the test name. You can provide a `name` property for the story to customize the test description, allowing you to write more descriptive names. This is useful when you want use spaces, brackets or other special characters in the test description.

@yannbf
Copy link
Member

yannbf commented Sep 20, 2024

Also, can you share examples of how you are naming your stories for your tests? It's pretty useful for us to have an idea about use cases! Thanks!

@InfiniteXyy
Copy link
Contributor Author

Also, can you share examples of how you are naming your stories for your tests? It's pretty useful for us to have an idea about use cases! Thanks!

Hi, thank you for your code review and feed back.
In our project, we are following a naming convention in test descriptions.

Each test cases should have a description like WHEN user click reply, THEN a request should be sent with a double confirm dialog.

It's more like a sentense, because we want each test to have a meaningful description that represents a specific scenario.

@InfiniteXyy
Copy link
Contributor Author

InfiniteXyy commented Sep 21, 2024

Hey @InfiniteXyy thanks a lot for your contribution! This was something we were planning to do, so thanks for speeding that up! I'd love if you could change the implemention to make it simpler, given that the underlying parsing tool (CsfTools) already handles extracting the story name.

I believe you can delete most of the code and use something like this:

const exportName = parsed._stories[exportName].name || exportName;
return getTestStatementForStory({
          exportName,
          node,
});

Test cases:

CSF1/2

export const Story = () => {}
Story.storyName = 'custom name'

CSF3

export const Story = () => {
  name: 'custom name'
}

Let me know how it goes and if you need anything else!

Thank you for your suggestions! It's helpful and by using parsedStory, the code looks just much better now.

I also try adding the test cases for csf v1/2 and function config, but I am not familiar with csf v1/2, hope I didn't do it wrong.

@yannbf yannbf changed the title Addon Vitest: support read story name for vitest test description Addon Test: Support story name as test description Sep 23, 2024
@yannbf
Copy link
Member

yannbf commented Sep 23, 2024

Hey @InfiniteXyy I'm trying to wrap this PR up but I am not allowed to push to your remote. Would you mind allowing edits from maintainers in your fork? Thanks!

@InfiniteXyy
Copy link
Contributor Author

Hey @InfiniteXyy I'm trying to wrap this PR up but I am not allowed to push to your remote. Would you mind allowing edits from maintainers in your fork? Thanks!

I have enabled this, hope it's the correct place for allowing permission
image

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Thank you so much for your contribution!

@yannbf yannbf enabled auto-merge September 23, 2024 15:52
@yannbf yannbf merged commit 0811ca8 into storybookjs:next Sep 23, 2024
52 checks passed
@github-actions github-actions bot mentioned this pull request Sep 23, 2024
21 tasks
@kasperpeulen kasperpeulen added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Sep 24, 2024
storybook-bot pushed a commit that referenced this pull request Sep 25, 2024
Addon Test: Support story name as test description

(cherry picked from commit 0811ca8)
@github-actions github-actions bot mentioned this pull request Sep 25, 2024
4 tasks
storybook-bot pushed a commit that referenced this pull request Sep 25, 2024
Addon Test: Support story name as test description

(cherry picked from commit 0811ca8)
storybook-bot pushed a commit that referenced this pull request Sep 26, 2024
Addon Test: Support story name as test description

(cherry picked from commit 0811ca8)
storybook-bot pushed a commit that referenced this pull request Sep 26, 2024
Addon Test: Support story name as test description

(cherry picked from commit 0811ca8)
storybook-bot pushed a commit that referenced this pull request Sep 27, 2024
Addon Test: Support story name as test description

(cherry picked from commit 0811ca8)
storybook-bot pushed a commit that referenced this pull request Sep 27, 2024
Addon Test: Support story name as test description

(cherry picked from commit 0811ca8)
@github-actions github-actions bot added the patch:done Patch/release PRs already cherry-picked to main/release branch label Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: test bug ci:normal patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants