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.

Sign up for GitHub

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

[Chromatic] Disable snapshots globally, only keep regression stories #1911

Closed
wants to merge 1 commit into from
Closed

[Chromatic] Disable snapshots globally, only keep regression stories #1911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wise-dragons-prove.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

[Chromatic] Disable snapshots globally, only keep regression stories
1 change: 0 additions & 1 deletion .github/workflows/storybook.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,3 @@ jobs:
token: ${{ secrets.GITHUB_TOKEN }}
projectToken: ${{ secrets.CHROMATIC_APP_CODE }}
autoAcceptChanges: "main"
onlyStoryFiles: "**/*regression.stories.tsx"
Copy link
Member

Choose a reason for hiding this comment

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

So I think the issue is that onlyStoryFiles is an option inside the chromatic.config.json file, the option is here: https://www.chromatic.com/docs/configure/#options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's good to know!

I think we're gonna enable a few key stories other than the regression stories soon, so I think it would still be worth removing this from the .yml file.

Copy link
Collaborator

@jeremywiebe jeremywiebe Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It seems like this is working as designed. This setting is documented as being supported in the GitHub Action:

image

On an example PR I checked, it seems like it is obeying the setting:

19:50:09.030 Starting partial build
19:50:09.030     → Snapshots will be limited to story files matching '**/*regression.stories.tsx'

...

19:50:58.649 Build 5134 passed!
                 → Tested 412 stories across 130 components; captured 27 snapshots in 39 seconds

It captured only 27 snapshots.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremywiebe It does say "captured 27 snapshots" on all the PRs. I checked a bunch of older unrelated Perseus PRs to check this, and it's true that that messaging is consistent. However, the PRs would also show other snapshots that aren't part of these 27 in the "UI test" check. It also says there are 130 tests on the chromatic dashboard for other PRs when there should only be 27:

image

I'd like to move away from using the onlyStoryFiles: "**/*regression.stories.tsx" because I think there's a disconnect somewhere that isn't super obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to retract my last comment. Just for a confidence check, I put up another PR that would have had a ton of UI test updates if my assertion was correct. It only reported changes in the regression stories, so I think my statements before this were all false, and I just don't know what "tests" means on the chromatic dashboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeremywiebe we can still continue with this PR if we want to enable other stories to have snapshots later, rather than just limiting to regression stories.

15 changes: 14 additions & 1 deletion .storybook/preview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as React from "react";

Check warning on line 1 in .storybook/preview.tsx

View workflow job for this annotation

GitHub Actions / Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x) 

File ignored by default.
import {color} from "@khanacademy/wonder-blocks-tokens";
import {RenderStateRoot} from "@khanacademy/wonder-blocks-core";
import {Dependencies} from "@khanacademy/perseus";
Expand All @@ -23,7 +23,14 @@
(Story) => (
<RenderStateRoot>
<DependenciesContext.Provider value={storybookDependenciesV2}>
<Story />
{/* Most of our components have an expectation to be
rendered inside of a .framework-perseus container.
We want to make sure we can include it here, since it
can also affect the styling.
*/}
<div className="framework-perseus">
<Story />
</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NOTE: This is causing some trivial snapshot updates within the UI tests for the regression stories.

</DependenciesContext.Provider>
</RenderStateRoot>
),
Expand Down Expand Up @@ -59,6 +66,12 @@
value,
})),
},
// Disabling Chromatic snapshots across all stories, since we have
// a limited snapshot budget. Set this explicitly to false on
// stories that we want snapshots for in their respective files.
chromatic: {
disableSnapshot: true,
},
},
};

Expand Down
12 changes: 11 additions & 1 deletion packages/perseus/src/widgets/interactive-graphs/interactive-graph-regression.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,22 @@ import {mockStrings} from "../../strings";
import {interactiveGraphQuestionBuilder} from "./interactive-graph-question-builder";

import type {PerseusRenderer} from "../../perseus-types";
import type {Meta} from "@storybook/react";

type StoryArgs = Record<any, any>;

export default {
const meta: Meta = {
title: "Perseus/Widgets/Interactive Graph Visual Regression Tests",
parameters: {
chromatic: {
// We want snapshots for these regression tests, so that
// visual differences can be caught if there are any
// unexpected changes.
disableSnapshot: false,
},
},
};
export default meta;

export const MafsWithCustomAxisLabels = (
args: StoryArgs,
Expand Down
Loading