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

Highlight trailing whitespace inside snapshots #2347

Merged

Conversation

thymikee
Copy link
Collaborator

Summary
Introduce highlighting of trailing whitespace. Kinda fixes #2287 I guess.

screen shot 2016-12-16 at 16 38 00

Test plan
jest

@codecov-io
Copy link

Current coverage is 89.23% (diff: 73.33%)

Merging #2347 into master will decrease coverage by 0.05%

@@             master      #2347   diff @@
==========================================
  Files            39         39          
  Lines          1475       1486    +11   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1317       1326     +9   
- Misses          158        160     +2   
  Partials          0          0          

Powered by Codecov. Last update 9acf3f6...cc82caf

@cpojer cpojer merged commit 3e61a49 into jestjs:master Dec 16, 2016
@SimenB
Copy link
Member

SimenB commented Dec 16, 2016

This looks good, thank you!

As an aside, I think it's really weird that new stuff is red with a plus. I get that it's the unexpected part so it's red, and it's an addition, so it's a plus, it just looks really weird.
I think it's because it's not what I expect from a diff-view. New/added stuff is green, deleted/old stuff is red. If I write the snapshot to disk, the git diff will invert the colours from what Jest shows me.

Should I create an issue for it? Maybe we can invert it, or at least provide an option? oldDiffColor: 'red', newDiffColor: 'green', or something with a much better name, would make me happy

@cpojer
Copy link
Member

cpojer commented Dec 16, 2016

We had long conversations about this with @DmitriiAbramov and @gaearon and decided to invert the colors, assuming that what is stored on disk is always the source of truth.

@SimenB
Copy link
Member

SimenB commented Dec 16, 2016

I find the diff view so confusing that I press u, and do a git diff instead... The overhead of reading a diff I have to concentrate to interpret is higher that pressing u and doing gd in another terminal.
And why should the source of (ultimate) truth matter? I care about what it was before vs what it is now. And the normal way of expressing that is old=red, new=green.

An option to invert the current schema would be greatly appreciated (I can provide a PR for it if you want?).

@aaronabramov
Copy link
Contributor

oh this is so nice!
this bug annoyed me for so long :)

@SimenB
Copy link
Member

SimenB commented Dec 16, 2016

My problem might be that the diff looks too much like a git diff, and I conflate the two when I look at it. I'm used to expected being green and actual being red. But when I look at hundreds of git diffs every day, having something be the same structure, but different colors, throws me off...

This is a diff like GH renders it:

diff --git i/preact/package.json w/preact/package.json
index 8d4aaab..0bbaf76 100644
--- i/preact/package.json
+++ w/preact/package.json
@@ -50,7 +50,7 @@
     "warning": "^3.0.0",
     "webpack": "^2.2.0-rc.0",
     "webpack-dev-middleware": "^1.8.4",
-    "webpack-dev-server": "^2.1.0-beta.12"
+    "webpack-dev-server": "^2.2.0-rc.0"
   },
   "jest": {
     "moduleFileExtensions": [
diff --git i/preact/yarn.lock w/preact/yarn.lock
index 1354866..8dfe46a 100644
--- i/preact/yarn.lock
+++ w/preact/yarn.lock
@@ -3174,7 +3169,7 @@ media-typer@0.3.0:
   version "0.3.0"
   resolved "http://npm.finntech.no/media-typer/-/media-typer-0.3.0.tgz#8710d7af0aa626f8fffa1ce00168545263255748"
 
-memory-fs@^0.4.0:
+memory-fs@^0.4.0, memory-fs@~0.4.1:
   version "0.4.1"
   resolved "http://npm.finntech.no/memory-fs/-/memory-fs-0.4.1.tgz#3a9a20b8462523e447cfbc7e8bb80ed667bfc552"
   dependencies:
@@ -4957,18 +4952,18 @@ webidl-conversions@^3.0.0, webidl-conversions@^3.0.1:
   version "3.0.1"
   resolved "http://npm.finntech.no/webidl-conversions/-/webidl-conversions-3.0.1.tgz#24534275e2a7bc6be7bc86611cc16ae0a5654871"
 
-webpack-dev-middleware@^1.4.0, webpack-dev-middleware@^1.8.4:
-  version "1.8.4"
-  resolved "http://npm.finntech.no/webpack-dev-middleware/-/webpack-dev-middleware-1.8.4.tgz#e8765c9122887ce9e3abd4cc9c3eb31b61e0948d"
+webpack-dev-middleware@^1.8.4, webpack-dev-middleware@^1.9.0:
+  version "1.9.0"
+  resolved "http://npm.finntech.no/webpack-dev-middleware/-/webpack-dev-middleware-1.9.0.tgz#a1c67a3dfd8a5c5d62740aa0babe61758b4c84aa"
   dependencies:
-    memory-fs "~0.3.0"
+    memory-fs "~0.4.1"
     mime "^1.3.4"
     path-is-absolute "^1.0.0"
     range-parser "^1.0.3"
 
-webpack-dev-server@^2.1.0-beta.12:
-  version "2.1.0-beta.12"
-  resolved "http://npm.finntech.no/webpack-dev-server/-/webpack-dev-server-2.1.0-beta.12.tgz#f717e7b69214dae0e7a2061c12d128432d7520ef"
+webpack-dev-server@^2.2.0-rc.0:
+  version "2.2.0-rc.0"
+  resolved "http://npm.finntech.no/webpack-dev-server/-/webpack-dev-server-2.2.0-rc.0.tgz#ea8a11e211d9524b8999945fe5645481a51fdf46"
   dependencies:
     chokidar "^1.6.0"
     compression "^1.5.2"
@@ -4983,7 +4978,7 @@ webpack-dev-server@^2.1.0-beta.12:
     spdy "^3.4.1"
     strip-ansi "^3.0.0"
     supports-color "^3.1.1"
-    webpack-dev-middleware "^1.4.0"
+    webpack-dev-middleware "^1.9.0"
     yargs "^6.0.0"
 
 webpack-hot-middleware@^2.13.2:

Going against that just seems weird to me. Buuuut if I could inverse it, I'd be happy 😀

@gaearon
Copy link
Contributor

gaearon commented Dec 16, 2016

I don't mind changing the snapshot colors but it needs to be done consistently.
Don't forget we have these views as well:

screen shot 2016-12-16 at 17 32 46

screen shot 2016-12-16 at 17 32 15

What colors do you propose for them? The second one also uses a diff so I would expect it to be colored the same way as snapshots. However this would make the "good" (expected) value red and the "bad" (actual) value green. Which, when also applied to the first screen, would be pretty confusing.

I agree it could be improved but I'd like to see a consistent proposal addressing all these points.
"Red is failure" is relatively easy to remember and currently works consistently.

@SimenB
Copy link
Member

SimenB commented Dec 16, 2016

Hmm, good point... Could introduce yellow and blue (2 other nice colours in chalk), but red/green is very common for tests.

Maybe changing the default is too much, but we can just make it configurable? Might be easier to settle on an alternative in the future if it could be tried out a bit.

Say you have colors, which takes options expected, received, diffExpected, diffReceived, and we go from there? (Diff can just prefix with bg for the trailing whitespace for now)

@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 13, 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.

Snapshot diff view doesn't render whitespace
7 participants