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

fix(jest-snapshot): always run prettier format a second time #11560

Merged
merged 4 commits into from
Sep 29, 2021
Merged

fix(jest-snapshot): always run prettier format a second time #11560

merged 4 commits into from
Sep 29, 2021

Conversation

G-Rath
Copy link
Contributor

@G-Rath G-Rath commented Jun 10, 2021

Summary

When writing inline snapshots, the custom prettier parser that handles indenting the snapshots is only called if there's a formatting difference between the original code vs with the new snapshot, which will only happen if the file needs to be formatted with prettier anyway.

This means if you already have code that is correctly formatted in prettier's eyes, we get a regression of #9477:

image

This wasn't caught by the test suite because (among other things), writeFiles calls dedent on all the file content its given which strips out all trailing and leading blank lines; this means that all files written by the suite using this method are considered not correctly formatted by prettier because it requires files have a trailing blank newline.

I've reproduced this by crafting a test whose file contents is considered correctly formatted, including with a trailing newline by using \\n to have dedent preserve that specific new line, and have fixed it by removing the formatting check - so now prettier.format is always called a second time to apply the snapshot parser/indenter.

Resolves #11459

Test plan

I've added a new test.

@codecov-commenter
Copy link

codecov-commenter commented Jun 10, 2021

Codecov Report

Merging #11560 (cc34849) into main (341843f) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #11560      +/-   ##
==========================================
- Coverage   68.73%   68.73%   -0.01%     
==========================================
  Files         322      322              
  Lines       16580    16577       -3     
  Branches     4784     4783       -1     
==========================================
- Hits        11397    11394       -3     
  Misses       5150     5150              
  Partials       33       33              
Impacted Files Coverage Δ
packages/jest-snapshot/src/InlineSnapshots.ts 90.60% <100.00%> (-0.19%) ⬇️

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 341843f...cc34849. Read the comment docs.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jun 17, 2021

@SimenB I managed to get it reproduced :D

I've restored the condition to confirm that the test fails on CI against the snapshot (which contains the correctly indented output) - will drop that commit after & restructure the code to call prettier twice.

I originally tried to do this using an object, but for some reason the sourceFileWithSnapshots kept having { a: '' } (instead of {a: ''}) for some reason, which would cause the conditional path to trigger. I couldn't shake that no matter what I tried, so there might technically be another bug in there but I think it might not actually be an issue - I was able to reproduce using arrays so for this PR it's now a non-issue 🤷

@G-Rath G-Rath marked this pull request as ready for review June 17, 2021 23:15
@G-Rath
Copy link
Contributor Author

G-Rath commented Jun 18, 2021

/Users/runner/work/jest/jest/packages/jest-repl/src/__tests__/runtime_cli.test.js

  ● Runtime CLI › throws script errors

    expect(received).toMatch(expected)

    Expected substring: "Error: throwing"
    Received string:    "Error: Cannot parse /Users/runner/work/jest/jest/e2e/console-log-output-when-run-in-band/package.json as JSON: ENOENT: no such file or directory, open '/Users/runner/work/jest/jest/e2e/console-log-output-when-run-in-band/package.json'
        at Object.worker (/Users/runner/work/jest/jest/packages/jest-haste-map/build/worker.js:146:13)
        at execFunction (/Users/runner/work/jest/jest/packages/jest-worker/build/workers/processChild.js:145:17)
        at execHelper (/Users/runner/work/jest/jest/packages/jest-worker/build/workers/processChild.js:124:5)
        at execMethod (/Users/runner/work/jest/jest/packages/jest-worker/build/workers/processChild.js:128:5)
        at process.messageListener (/Users/runner/work/jest/jest/packages/jest-worker/build/workers/processChild.js:46:7)
        at process.emit (events.js:376:20)
        at emit (internal/child_process.js:910:12)
        at processTicksAndRejections (internal/process/task_queues.js:83:21)"

      at Object.toMatch (packages/jest-repl/src/__tests__/runtime_cli.test.js:51:52)

😞 tho I don't think thats the result of my change.

@G-Rath
Copy link
Contributor Author

G-Rath commented Jul 6, 2021

@SimenB any chance of getting a quick review? 😅

@nfarina
Copy link
Contributor

nfarina commented Aug 12, 2021

Would love this merged so I can upgrade to Jest 27! Starting to use inline snapshots extensively and the lack of indentation is a deal breaker.

@G-Rath
Copy link
Contributor Author

G-Rath commented Sep 3, 2021

@SimenB would be great if we could land this 🙂

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.

Thanks!

And sorry about the delay

@SimenB SimenB merged commit 6b18aed into jestjs:main Sep 29, 2021
@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 Oct 30, 2021
@G-Rath G-Rath deleted the fix-snapshot-indenting branch October 30, 2021 00:56
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.

Inline snapshots are indented at col 0
5 participants