feat(snapshot): concatenate name of test and snapshot

[Haroenv]

Summary

I find the context of the test name and the snapshot name very useful in cases where you have multiple different

Meanwhile I also added the option to add a testName to toThrowErrorMatchingSnapshot.

I inspired from #2094, but couldn't really find back where the exact tests are I need to change for this to work.

Let me know if I'm on the correct path

Test plan

I should change the tests for toMatchSnapshot, but I'm not totally sure which exactly test this

[thymikee]

Can you explain why it is removed?

[Haroenv]

because expected now is the testName

[Haroenv]

I have a bunch of failing tests running locally that seem to be unrelated to this change, so if someone can take a look in what causes these failures locally, as well as on the CI

Snapshot Summary
 › 64 snapshot tests failed in 16 test suites. Inspect your code changes or run with `yarn run jest -- -u` to update them.

Test Suites: 17 failed, 149 passed, 166 total
Tests:       63 failed, 1512 passed, 1575 total
Snapshots:   64 failed, 625 passed, 689 total
Time:        75.757s
Ran all test suites.
error Command failed with exit code 1

[SimenB]

@Haroenv Your new test fails on appveyor, could you take a look?

[Haroenv]

@SimenB, I don't understand how the test can fail on appVeyor but not on other platforms. It doesn't seem to be anything windows-specific?

[SimenB]

If you still have issues locally, try rebasing on master, and doing yarn clean-all && yarn && yarn test. Ensure you are on [email protected] as well.

I do agree it seems odd the test should only fail on appveyor, though. Maybe the generated filename somehow is messed up? I'd be interested in a fresh CI run, so if you could push a rebased PR, that'd be great. Restarting the node 6 build on circle didn't help...

[Haroenv]

Tests have always passed on my end

[Haroenv]

@SimenB tests pass now, but node 4 fails from the merge 🤔

[SimenB]

That is my fault, I activated a test in #4631 that uses Array.prototype.includes, which is not supported on node 4.

The CircleCI badge should not be green, I have no idea what it looks at.

EDIT: #4640

[codecov-io]

Codecov Report

Merging #4460 into master will increase coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #4460      +/-   ##
==========================================
+ Coverage   57.12%   57.13%   +<.01%     
==========================================
  Files         194      194              
  Lines        6566     6565       -1     
  Branches        3        3              
==========================================
  Hits         3751     3751              
+ Misses       2815     2814       -1

Impacted Files	Coverage Δ
packages/jest-snapshot/src/index.js	43.18% <50%> (+0.95%)	⬆️

---

Continue to review full report at Codecov.

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

[cpojer]

Wait, can somebody explain this change to me? I don't get it.

[Haroenv]

My idea was that if you're adding a custom snapshot name, it is because you have a lot of them in the same test and want to reference which is the test it actually occurs in. Imagine having a few different cases in which you'll add the same snapshots, then you might want to know which snapshot is for which test.

const getData = format => ({
  color: 'red',
  format,
});

it('works with format true', () => {
  const data = getData(true);
  expect(Object.keys(data)).toMatchSnapshot('has correct keys');
  expect(data.format).toMatchSnapshot('has correct format')
});

it('works with format false', () => {
  const data = getData(false);
  expect(Object.keys(data)).toMatchSnapshot('has correct keys');
  expect(data.format).toMatchSnapshot('has correct format')
});

This is a bit simpler than what you would be doing in real life, but any repeated test cases under similar use cases would make sense.

The problem with the implementation right now, is that when you look at the snapshot file, you don't know which test a certain assertion is coming from.

Obviously you can work around this before this PR by giving them separate names, but that seems a bit tedious to me.

[cpojer]

Let's do it, although it's a breaking change.

[Haroenv]

Thanks :)

[github-actions]

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.