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

ci(screener): clean up and exclude unnecessary stories #5257

Merged
merged 21 commits into from
Sep 7, 2022

Conversation

benelan
Copy link
Member

@benelan benelan commented Sep 1, 2022

Related Issue: #5253

Summary

This PR cleanups up and combines stories to lower our snapshot count below 40k per month.

  • Append _NoTest to story names that we don't want screener to snapshot.
  • Append _TestOnly to story names that we do not want to appear in the Storybook deployment on github pages.
  • Set up filters to accomplish the above two points. The filter determines which stories to skip using a boolean environment variable: STORYBOOK_SCREENSHOT_TEST_BUILD. The default is NoTest, and the env var is set to true in the pr-screener workflow.
  • Combine stories, e.g. if a component has "DarkTheme" and "RTL" snapshots, we combined them into a single darkThemeRTL_TestOnly snapshot.
  • Consistency cleanups, including camel case story names and using "simple" as the name for the first story (rather than "basic").

Copy link
Member

@jcfranco jcfranco left a comment

Choose a reason for hiding this comment

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

✨🧹📚✨

@benelan benelan marked this pull request as ready for review September 7, 2022 20:03
@benelan benelan requested a review from a team as a code owner September 7, 2022 20:03
@benelan benelan marked this pull request as draft September 7, 2022 22:54
@benelan benelan marked this pull request as ready for review September 7, 2022 22:54
@benelan benelan merged commit a1bef22 into master Sep 7, 2022
@benelan benelan deleted the benelan/prune-stories branch September 7, 2022 23:11
eriklharper pushed a commit that referenced this pull request Sep 8, 2022
**Related Issue:** #5257

## Summary
Some additional cleanup. I nuked a lot of the combinations but we can
add some back once we are out of crisis mode
<!--

Please make sure the PR title and/or commit message adheres to the
https://www.conventionalcommits.org/en/v1.0.0/ specification.

Note: If your PR only has one commit and it is NOT semantic, you will
need to either

a. add another commit and wait for the check to update
b. proceed to squash merge, but make sure the commit message is the same
as the title.

This is because of the way GitHub handles single-commit squash merges
(see zeke/semantic-pull-requests#17)

If this is component-related, please verify that:

- [ ] feature or fix has a corresponding test
- [ ] changes have been tested with demo page in Edge

---

If this is skipping an unstable test:

- include info about the test failure
- submit an unstable-test issue by
[choosing](https://github.com/Esri/calcite-components/issues/new/choose)
the unstable test template and filling it out

-->
dir="${select("dir", ["ltr", "rtl"], "ltr", "Tile Select Group")}"
class="calcite-theme-dark"
>
const tileSelectsHTML = html`
Copy link
Member

Choose a reason for hiding this comment

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

@benelan this needs to return a function. Otherwise, the knobs get applied to every story.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco is this payback for calling out the others, are you trying to pin this one on me??

nice-try

Copy link
Member

Choose a reason for hiding this comment

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

Sorry @benelan I just saw it was merged in that PR which was yours :)

Copy link
Member Author

Choose a reason for hiding this comment

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

lol all good, it was a just friendly jab at Franco because he's been having a rough time exporting stories, e.g. here, here, here

Franco right now:
snitch-not-absent

Copy link
Member

Choose a reason for hiding this comment

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

Franco is now banned from touching stories :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants