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

Support custom inline snapshot matchers #9278

Merged
merged 14 commits into from Dec 8, 2019
Merged

Support custom inline snapshot matchers #9278

merged 14 commits into from Dec 8, 2019

Conversation

kevin940726
Copy link
Contributor

@kevin940726 kevin940726 commented Dec 7, 2019

Fix #8181.

Summary

Supports custom inline snapshot matchers, currently using them isn't possible.

Test plan

const { toMatchInlineSnapshot } = require("jest-snapshot");

expect.extend({
  toMatchCustomInlineSnapshot(received, ...args) {
    return toMatchInlineSnapshot.call(this, received, ...args);
  }
});

test("inline snapshots", () => {
  expect({ apple: "original value" }).toMatchCustomInlineSnapshot();
  // Will inject snapshot to
  expect({ apple: "original value" }).toMatchCustomInlineSnapshot(`
    Object {
      "apple": "original value",
    }
  `);
});

How

The implementation is a bit of a hack, I'm not sure if it's suitable for production but it's the only better way I can think of for now.

To summarize the issue to those who is not familiar with how inline snapshots works, here is a simplified breakdown.

  1. snapshots stack trace is generated upon runtime call, by calling new Error().stack
  2. In saveSnapshotsForFile function, it takes the snapshots coming from the previous step and the sourceFilePath
expect(something).toMatchInlineSnapshot();
  1. It inserts the snapshots if necessary to the source file in createInsertionParser
expect(something).toMatchInlineSnapshot(`"SNAPSHOT...
NEXT LINE"`);
  1. It formats the source file with the added snapshots with prettier
expect(something).toMatchInlineSnapshot(`
"SNAPSHOT...
NEXT LINE"
`);
  1. The indention is not yet correct, so it indents the snapshots to make them look good with the source again in createFormattingParser
expect(something).toMatchInlineSnapshot(`
  "SNAPSHOT...
  NEXT LINE"
`);
  1. It formats the source file yet again to make the overall indention looks good
expect(something).toMatchInlineSnapshot(`
  "SNAPSHOT...
  NEXT
  LINE"
`);
  1. It overrides the file when done

There's a tweet mentioned in the original issue stating that we should need to be able to figure out that the call is coming from a custom matcher and skip those frames in order for this to work. So this is exactly what I've done. I create a no-op trap for every external matcher, wrapping them with a function call so that it would show up in the stack trace.

The next step is to modify the logic where we get the top frame. We remove all stack trace lines before the trap function we just made (if it exists), so that when we try to get the top frame it won't get the native toMatchInlineSnapshot but the custom toMatchCustomInlineSnapshot.

We also have to update the babel traverse logic in createFormattingParser, or else it won't correctly indent the snapshots. Currently it only looks for matcher named toMatchInlineSnapshot, I modify it to record all the matcher names we've seen in the stack traces, and check them one by one in the parser.


I added an e2e test case to demonstrate the feature, please tell me if you have any other idea for tests, or edge cases I'm missing.

I'm also aware of that #7792 is near to be finished, this PR takes it into account and should be able to work just fine when it landed.

I'll update the CHANGELOG file once if you think it's a good idea :)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

This is really nice, thanks! Didn't turn out as hacky as I feared

packages/expect/src/index.ts Show resolved Hide resolved
@@ -104,7 +105,9 @@ export default class SnapshotState {
this._dirty = true;
if (options.isInline) {
const error = options.error || new Error();
const lines = getStackTraceLines(error.stack || '');
const lines = getStackTraceLines(
removeLinesBeforeExternalMatcherTrap(error.stack || ''),
Copy link
Member

@SimenB SimenB Dec 7, 2019

Choose a reason for hiding this comment

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

Should getTopFrame do this work internally instead of the caller having to do it? It seems like the behaviour we'd always want, no?

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 think the stack trace containing __EXTERNAL_MATCHER_TRAP__ will already got removed by getStackTraceLines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we instead modify getStackTraceLines? Or both?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, good point. Is it enough to just change https://github.com/facebook/jest/blob/9ac2dcd55c0204960285498c590c1aa7860e6aa8/packages/jest-message-util/src/index.ts#L49 to not filter out this new capture line? If not I think the approach you have here makes sense

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 tried, it works fine in this context, but it would break in formatStackTrace.

https://github.com/facebook/jest/blob/9ac2dcd55c0204960285498c590c1aa7860e6aa8/packages/jest-message-util/src/index.ts#L254-L255

If we omit all stack traces before the trap, then the custom matcher won't correctly format the stack trace. It's covered here in an e2e test.

https://github.com/facebook/jest/blob/9ac2dcd55c0204960285498c590c1aa7860e6aa8/e2e/custom-matcher-stack-trace/__tests__/sync.test.js#L41-L54

I believe that we have 2 options:

  1. Leave it be like this to only remove stack traces from the caller
  2. Add logic in formatStackTrace to un-skip the trap.

I think (1) is more resilient while (2) feel more error-prone. Wonder what's your opinion on this?

packages/jest-snapshot/src/utils.ts Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you! Just updating the changelog and we should be good then

@kevin940726
Copy link
Contributor Author

kevin940726 commented Dec 7, 2019

@SimenB Awesome! Thanks a lot! Should I also update the doc to include a section about custom inline snapshot matcher?

@SimenB
Copy link
Member

SimenB commented Dec 7, 2019

A docs update would be wonderful, yes!

@SimenB
Copy link
Member

SimenB commented Dec 7, 2019

/cc @azz

CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB SimenB merged commit 012fc8b into jestjs:master Dec 8, 2019
@azz
Copy link
Contributor

azz commented Dec 8, 2019

Thanks so much for fixing this @kevin940726 🥇

@Haroenv
Copy link
Contributor

Haroenv commented Dec 9, 2019

This looks amazing, It's been on my watchlist for a year!

@kevin940726 kevin940726 deleted the custom-inline-snapshot-matcher branch December 9, 2019 13:06
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getTopFrame not returning the top frame when creating a custom inline snapshot matcher
5 participants