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

Allow primitives, objects, arrays and functions as arguments to toMatchInlineSnapshot #9890

Closed
MarcoScabbiolo opened this issue Apr 26, 2020 · 11 comments

Comments

@MarcoScabbiolo
Copy link
Contributor

MarcoScabbiolo commented Apr 26, 2020

🚀 Feature Proposal

Make toMatchInlineSnapshot able to handle primitive values (strings, booleans, null, undefined, numbers), objects (deep), arrays (deep), functions (check if it is a function).

Motivation

This is heavily influenced by @aaz's comment in #7792 (comment) and the motivation is to have more powerful inline snapshots by having the actual values in the code instead of strings representations of them.

Example

expect({
    a: 1,
    b: "2",
    c: true,
    d: null,
    e: undefined,
    f: [1, "2", [false]],
    g: () => {}
  }).toMatchInlineSnapshot({
    a: 1,
    b: "2",
    c: true,
    d: null,
    e: undefined,
    f: [1, "2", [false]],
    g: expect.any(Function)
  });

Pitch

The example is the pitch, toMatchInlineSnapshot is a very powerful tool, and it can do a lot better than just printing strings. Get the data you are trying to snapshot in front of you by just running the tests and change it accordingly writing JS instead of JS in a string.

`
    Object {
      "a": 1,
      "b": "2",
      "c": true,
      "d": null,
      "e": undefined,
      "f": Array [
        1,
        "2",
        Array [
          false,
        ],
      ],
      "g": [Function],
    }
  `

vs

{
    a: 1,
    b: "2",
    c: true,
    d: null,
    e: undefined,
    f: [1, "2", [false]],
    g: expect.any(Function)
  }

Implementation

Probably will have to be done on top of #7792 , it is good that babel is already being used in that PR as it is the most powerful AST editor out there.

I'd be glad to implement this myself and ideally have it ready for Jest 26.

@SimenB
Copy link
Member

SimenB commented Apr 26, 2020

Oh yeah, this is the dream! 😀 If you have any good ideas on how to do this, I'd love to try to get it in 👍 We most definetely want this - snapshots (inline or not) being strings instead of values is a technical limitation, not something we necessarily want (although we should probably keep it as an option for folks)

@MarcoScabbiolo
Copy link
Contributor Author

I'll try to get it working using inline snapshots starting from #7792, and if it looks good then we can discuss the details about breaking changes, regular snapshots and related options/configs.

@jeysal
Copy link
Contributor

jeysal commented Apr 26, 2020

If you want to help debug the OOM problem on #7792 so that we can land it feel free 😂 (half serious 😅 )

@SimenB
Copy link
Member

SimenB commented Apr 26, 2020

Mem leak issue, fwiw: #8816

Might be worth sticking a bounty on it...

@MarcoScabbiolo
Copy link
Contributor Author

MarcoScabbiolo commented May 27, 2020

I've done some progress on this, I'm getting expected failures in existing tests:

Running node_modules/.bin/jest e2e/__tests__/toMatchInlineSnapshot.test.ts

I get stuff like:

- Snapshot  - 9
+ Received  + 1

-
- "
  it('works with inline snapshots', () => {
-   expect({a: 1}).toMatchInlineSnapshot(`
-     Object {
-       \"a\": 1,
-     }
-   `);
+   expect({a: 1}).toMatchInlineSnapshot({a: 1});
  });
-
- "

or

- Snapshot  - 8
+ Received  + 2

-
- "
  test('inline snapshots', async () => {
-   expect(Promise.resolve('success')).resolves.toMatchInlineSnapshot(
-     `\"success\"`,
-   );
-   expect(Promise.reject('fail')).rejects.toMatchInlineSnapshot(`\"fail\"`);
+   expect(Promise.resolve('success')).resolves.toMatchInlineSnapshot('success');
+   expect(Promise.reject('fail')).rejects.toMatchInlineSnapshot('fail');
  });
-
- "

or

- Snapshot  - 5
+ Received  + 1

-
- "
  test('inline snapshots', async () => {
    await 'next tick';
-   expect(42).toMatchInlineSnapshot(`42`);
+   expect(42).toMatchInlineSnapshot(42);
  });
-
- "

I was thinking this would be the ideal behaviour and there should exist an option to opt-out of this new feature, something like --serialized-snapshots. @SimenB do you think this should be the case? If so, I can route this option to expects context and pick it up in jest-snapshot from the context to differentiate serialized (old) snapshots from new ones. Then I change all existing tests to use serialized snapshots and create the new tests.

Right now I'm working only on inline snapshots, for in-file or "regular" snapshots I think using a .js file that exports a string-to-snapshot hashmap (object) would be ideal, but we can discuss that later if this progresses.

Here is the branch if you want to check it out: https://github.com/MarcoScabbiolo/jest/tree/9890-snapshots it is based on #7792

@jeysal
Copy link
Contributor

jeysal commented May 28, 2020

I would think this behavior is nicer for everyone without disadvantages, so that we don't need an opt-out flag. For backward compatibility, maybe we could build it so that without -u, the serialized string version is still accepted as correct, but with -u it is updated to be a non-string. I think this is similar to how indentation already behaves - there are multiple serialized strings accepted for the snapshot with different indentations, but when it updates it writes the one with the prettiest indentation.

@MarcoScabbiolo
Copy link
Contributor Author

The only bad thing about the backwards compatibility is having to double check every failed snapshot every time, once with the new method and again with the serialized method.

@jeysal
Copy link
Contributor

jeysal commented May 28, 2020

I think that's fine, failing assertions are not a hot path we need to optimize, even less so than passing assertions. Printing the message/diff is already expensive.

@github-actions
Copy link

This issue is stale because it has been open for 1 year with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 25, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 7 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions
Copy link

github-actions bot commented May 4, 2022

This issue 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 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants