Skip to content

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

Merged
SimenB merged 4 commits into
jestjs:mainfrom
G-Rath:fix-snapshot-indenting
Sep 29, 2021
Merged

fix(jest-snapshot): always run prettier format a second time#11560
SimenB merged 4 commits into
jestjs:mainfrom
G-Rath:fix-snapshot-indenting

Conversation

@G-Rath

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

Copy link
Copy Markdown
Contributor

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

codecov-commenter commented Jun 10, 2021

Copy link
Copy Markdown

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

G-Rath commented Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

@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

G-Rath commented Jun 18, 2021

Copy link
Copy Markdown
Contributor Author
/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

G-Rath commented Jul 6, 2021

Copy link
Copy Markdown
Contributor Author

@SimenB any chance of getting a quick review? 😅

@nfarina

nfarina commented Aug 12, 2021

Copy link
Copy Markdown
Contributor

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

G-Rath commented Sep 3, 2021

Copy link
Copy Markdown
Contributor Author

@SimenB would be great if we could land this 🙂

@SimenB SimenB left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown

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