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

Replace interactive graph snapshot tests with Storybook stories #1289

Merged
merged 3 commits into from
May 17, 2024

Conversation

benchristel
Copy link
Member

@benchristel benchristel commented May 16, 2024

Summary:

There is a problem with using Jest snapshot tests to test Mafs: Mafs
uses globally autoincrementing IDs for some of its elements, so when
new tests are added to a suite, the IDs shift and all later snapshot
tests fail.

To work around this, I've removed the problematic Jest snapshot tests
and replaced them with Storybook stories.

These stories will be run as visual snapshot tests on Chromatic,
providing similar regression protection with hopefully less test
flakiness.

Issue: LEMS-1975

Test plan:

Watch CI run and accept the baselines for the new stories on Chromatic.

There is a problem with using Jest snapshot tests to test Mafs: Mafs
uses autoincrementing IDs for some of its elements, so when new tests
are added to a suite, the IDs change and the snapshot tests fail.

To work around this, I've removed the problematic Jest snapshot tests
and replaced them with Storybook stories.

These stories will be run as visual snapshot tests on Chromatic,
providing similar regression protection with hopefully less test
flakiness.

Issue: LEMS-1975

Test plan:

Watch CI run and accept the baselines for the new stories on Chromatic.
@khan-actions-bot
Copy link
Contributor

khan-actions-bot commented May 16, 2024

Gerald

Required Reviewers
  • @Khan/perseus for changes to .changeset/heavy-hats-sleep.md, packages/perseus/src/widgets/__stories__/interactive-graph-regression.stories.tsx, packages/perseus/src/widgets/__tests__/interactive-graph.test.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.test.ts, packages/perseus/src/widgets/interactive-graphs/interactive-graph-question-builder.ts, packages/perseus/src/widgets/__tests__/__snapshots__/interactive-graph.test.ts.snap

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

@khan-actions-bot khan-actions-bot requested a review from a team May 16, 2024 23:03
Copy link
Contributor

github-actions bot commented May 16, 2024

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (2e49ecb) and published it to npm. You
can install it using the tag PR1289.

Example:

yarn add @khanacademy/perseus@PR1289

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR1289

Copy link
Contributor

github-actions bot commented May 16, 2024

Size Change: 0 B

Total Size: 839 kB

ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 38.1 kB
packages/kmath/dist/es/index.js 4.27 kB
packages/math-input/dist/es/index.js 80.5 kB
packages/math-input/dist/es/strings.js 1.73 kB
packages/perseus-core/dist/es/index.js 908 B
packages/perseus-editor/dist/es/index.js 268 kB
packages/perseus-error/dist/es/index.js 877 B
packages/perseus-linter/dist/es/index.js 21.8 kB
packages/perseus/dist/es/index.js 403 kB
packages/perseus/dist/es/strings.js 3.22 kB
packages/pure-markdown/dist/es/index.js 3.68 kB
packages/simple-markdown/dist/es/index.js 12.4 kB

compressed-size-action

Copy link
Contributor

@Myranae Myranae left a comment

Choose a reason for hiding this comment

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

Looks good~ I see the regression tests in the UI Tests area of the PR checks. Also the publish to chromatic step was successful! So after this, the goal is for everyone to not make any more Jest snapshot tests but to instead use these Storybook regression tests, right? Also, looks like some checks are failing, but they don't seem to be related to this PR specifically.
image
image

@benchristel
Copy link
Member Author

the goal is for everyone to not make any more Jest snapshot tests but to instead use these Storybook regression tests, right?

You can write Jest snapshot tests for individual components. It's just the main MafsGraph component that's the problem — MafsGraph renders the axis grid, which uses autoincrementing IDs.

Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.29%. Comparing base (5b52a15) to head (2e49ecb).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1289      +/-   ##
==========================================
+ Coverage   69.11%   70.29%   +1.18%     
==========================================
  Files         476      481       +5     
  Lines      101779   101908     +129     
  Branches     7269    10929    +3660     
==========================================
+ Hits        70341    71634    +1293     
+ Misses      31259    30274     -985     
+ Partials      179        0     -179     

Impacted file tree graph

Files Coverage Δ
...ctive-graphs/interactive-graph-question-builder.ts 100.00% <100.00%> (ø)

... and 146 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b52a15...2e49ecb. Read the comment docs.

@jeremywiebe
Copy link
Collaborator

I would love (or would have loved) to find a different solution for this. This means folks now have to be aware that some tests can use Jest snapshot tests and some Storybook visual snapshot tests.

I know Jest supports custom Property Matchers for snapshots, but I haven't seen how to configure this globally.

@benchristel benchristel merged commit 42c0c60 into main May 17, 2024
13 checks passed
@benchristel benchristel deleted the benc/rethink-snapshots branch May 17, 2024 17:00
benchristel pushed a commit that referenced this pull request May 21, 2024
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.


# Releases
## @khanacademy/perseus@22.6.0

### Minor Changes

-   [#1293](#1293) [`e14a003be`](e14a003) Thanks [@benchristel](https://github.com/benchristel)! - Fill Mafs interactive circles with blue on hover


-   [#1241](#1241) [`a0dfc66cc`](a0dfc66) Thanks [@SonicScrewdriver](https://github.com/SonicScrewdriver)! - New Axis Tick Labels and Ticks that can render outside of graph bounds

### Patch Changes

-   [#1289](#1289) [`42c0c607f`](42c0c60) Thanks [@benchristel](https://github.com/benchristel)! - Internal: replace some brittle SVG snapshot tests with less brittle visual snapshot tests


-   [#1271](#1271) [`55039a430`](55039a4) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Bugfix: Arrowhead Rotation Was Incorrect on Some Graphs


-   [#1295](#1295) [`f6be03dd8`](f6be03d) Thanks [@benchristel](https://github.com/benchristel)! - Fix a bug where the arrow at the end of a line or ray would sometimes point to the origin and not the edge of the graph


-   [#1294](#1294) [`fba227fe8`](fba227f) Thanks [@benchristel](https://github.com/benchristel)! - Fix a bug where axis tick labels on Mafs interactive graphs could be selected and interfere with drag interactions


-   [#1255](#1255) [`dc0adedeb`](dc0aded) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Ensure that axis lines and arrowheads have a 2px thickness in Interactive Graphs


-   [#1264](#1264) [`d70fab6a7`](d70fab6) Thanks [@mark-fitzgerald](https://github.com/mark-fitzgerald)! - Show Radio Widget Images on New Line


-   [#1285](#1285) [`5b52a1569`](5b52a15) Thanks [@benchristel](https://github.com/benchristel)! - Internal: refactor the code for initializing linear graph states

## @khanacademy/perseus-editor@6.5.0

### Minor Changes

-   [#1277](#1277) [`f8539c880`](f8539c8) Thanks [@nishasy](https://github.com/nishasy)! - Shows error in the editor if locked line has length 0


-   [#1284](#1284) [`8534a9f01`](8534a9f) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Add ToggleableCaret component and use in TexErrorView

### Patch Changes

-   [#1287](#1287) [`d9b51dcc0`](d9b51dc) Thanks [@jeremywiebe](https://github.com/jeremywiebe)! - Interactive Graph Editor: Make the common graph settings a collapsable panel


-   [#1278](#1278) [`fffd130db`](fffd130) Thanks [@nishasy](https://github.com/nishasy)! - Nit: put the line kind dropdown options in alphabetical order


-   [#1280](#1280) [`5b1e04abc`](5b1e04a) Thanks [@nishasy](https://github.com/nishasy)! - Fix the bug where the first added locked figure settings would be collapsed when it's supposed to be expanded on add

-   Updated dependencies \[[`e14a003be`](e14a003), [`42c0c607f`](42c0c60), [`55039a430`](55039a4), [`f6be03dd8`](f6be03d), [`fba227fe8`](fba227f), [`dc0adedeb`](dc0aded), [`a0dfc66cc`](a0dfc66), [`d70fab6a7`](d70fab6), [`5b52a1569`](5b52a15)]:
    -   @khanacademy/perseus@22.6.0

## @khanacademy/perseus-dev-ui@1.5.8

### Patch Changes

-   [#1291](#1291) [`cceca19c7`](cceca19) Thanks [@benchristel](https://github.com/benchristel)! - Update dependency versions


-   [#1290](#1290) [`d41feb9be`](d41feb9) Thanks [@benchristel](https://github.com/benchristel)! - Internal: upgrade to @khanacademy/wonder-blocks-timing@5 in dev UI

Author: khan-actions-bot

Reviewers: benchristel

Required Reviewers:

Approved By: benchristel

Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️  Publish npm snapshot, ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1279
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants