Please see this comment: https://github.com/avajs/ava/issues/1558#issuecomment-469031454
Original issue follows.
Description
When using t.snapshot(foo) and foo does not match the snapshot, the diff that is shown uses - to indicate the new foo values and + to show the old foo values.
This is unintuitive.
I believe the problem may be at https://github.com/avajs/ava/blob/14f7095d25abc5ffbff7efd7db962eaf5e86daab/lib/assert.js#L321
Test Source
import test from 'ava'
test('snapshot diff', t => {
t.snapshot(`\n${Date.now()}\n`, 'the - value should be less than the + value')
})
Environment
Latest stable ava on OS X
IssueHunt Summary
Backers (Total: $60.00)
issuehunt ($60.00)
Submitted pull Requests
- #2199 Added gutter descriptors
Become a backer now!
Or submit a pull request to get the deposits!
Tips
- Checkout the Issuehunt explorer to discover more funded issues.
- Need some help from other developers? Add your repositories on IssueHunt to raise funds.
IssueHunt has been backed by the following sponsors. Become a sponsor
wmertens
All 18 comments
The idea is that the new value should be adjusted to match the old value. If you'd used t.is():
direct diff
/private/var/folders/2_/qczp184x76b2nl034sq5hvxw0000gn/T/tmp.dcvW5BUyA5/test.js:8
7: test('direct diff', t => {
8: t.is(`\n${Date.now() - 42e5}\n`, `\n${Date.now()}\n`)
9: })
Difference:
`␊
- 1508765718717␊
+ 1508769918717␊
`
What is definitely wrong though is that the colors are not inverted the way the gutters are. I've just opened https://github.com/concordancejs/concordance/issues/40 for that.
If that were fixed would you be happy with the current behavior?
novemberborn
on 23 Oct 2017
Well, the colors are the primary indicator for me, but I am so used to the
patch view that reversing the order is odd to me, - means "previous", not
"wrong value that you should fix".
On Mon, Oct 23, 2017, 4:50 PM Mark Wubben notifications@github.com wrote:
The idea is that the new value should be adjusted to match the old value.
If you'd used t.is():direct diff
/private/var/folders/2_/qczp184x76b2nl034sq5hvxw0000gn/T/tmp.dcvW5BUyA5/test.js:8
7: test('direct diff', t => {
8: t.is(\n${Date.now() - 42e5}\n,\n${Date.now()}\n)
9: })Difference:
`␊- 1508765718717␊
+ 1508769918717␊
`What is definitely wrong though is that the colors are not inverted the
way the gutters are. I've just opened concordancejs/concordance#40
https://github.com/concordancejs/concordance/issues/40 for that.If that were fixed would you be happy with the current behavior?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/avajs/ava/issues/1558#issuecomment-338684389, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADWlqOsoB5v_2zAw7RumpYqfpqOobomks5svKe0gaJpZM4QAMuE
.
wmertens
on 23 Oct 2017
In the t.is(actual, expected) example below, do the indicators make sense?
The - value is supposed to be "bad", with the + value being "good." That should then translate to the snapshot, where the current value is assumed to be "bad", since it doesn't match the "good" snapshot.
Just like a code patch in a bugfix takes you from a "bad" file to a "good" file, so would "applying" the diff in AVA's output help you make the actual value match the expectation. I think that holds for snapshots, too.
novemberborn
on 23 Oct 2017
I have to agree with @wmertens, this is counter intuitive.
Reason being that Ava has this nifty feature $ ava --update-snapshots which at least in my mind is analogous to git commit or git push.
With this in mind I find it more intuitive that the diff shows what will happen if the snapshot is updated.
Which means the original should be denoted by a minus and in red, while the new (updated) value should be denoted by a plus and in green.
But that's just my opinion.
Edit:
Looking at the jest snapshot testing documentation it seems they're using a compromise:
- The symbol (minus or plus) indicates the history of the value, minus being the original value, and plus being the new (updated) one
- The color (red or green) indicates the (assumed) correctness of the value, red being incorrect, and green, correct
They also explicitly state which is which to avoid any confusion.
I think this may be the best approach.
nesbocaj
on 22 Nov 2017
@nesbocaj so if I'm interpreting this right, we'd keep the - and + as they are now but make the - lines green and the + lines red?
novemberborn
on 28 Nov 2017
@novemberborn I'm having a hard time figuring out what the current order of the values are, so just to avoid any confusion I'm gonna lay it out below:
- the snapshot value, colored in green
+ the new (updated) value, colored in red
Doing it this way will make Ava more approachable to developers coming from Jest and vice versa, as well as work towards establishing some kind of standard in the matter.
nesbocaj
on 28 Nov 2017
@novemberborn It is worth noting however that this way of doing it is still different from what most developers will be familiar with from eg. git diffs:
- the snapshot value
+ the new (updated) value
So I think it would be beneficial to have some kind of introduction to the diff syntax, akin to how Jest does it:
Of course the lines
Received value does not match stored snapshot 1.
- Snapshot
+ Received
do take up quite a bit of screen real estate and it's up to the Ava developers as to whether or not they want to make this compromise.
Maybe it should only be shown to new developers, eg. the first 5 or so times a failed snapshot test is encountered, but I'm not sure if this is even possible.
nesbocaj
on 28 Nov 2017
@nesbocaj makes sense. @wmertens what do you think?
novemberborn
on 28 Nov 2017
@novemberborn any update on this? I have been tripped up by the + / - difference with AVA so many times.
I think what @nesbocaj said makes sense. For me the colors are not as important as the - / + and since snapshots are tests that are just diffing 2 values, it make the most sense to have the - be the existing value in the snapshot and the + be the new value from the test (I have not opinion on the colors, probably makes to to just match what jest does).
rzec-r7
on 1 Mar 2018
Yes, the way Jest does it looks good!
On Thu, Mar 1, 2018 at 12:53 PM rzec-r7 notifications@github.com wrote:
@novemberborn https://github.com/novemberborn any update on this? I
have been tripped up by the + / - difference with AVA so many times.I think what @nesbocaj https://github.com/nesbocaj said makes sense.
For me the colors are not as important as the - / + and since snapshots
are tests that are just diffing 2 values, it make the most sense to have
the - be the existing value in the snapshot and the + be the new value
from the test (I have not opinion on the colors, probably makes to to just
match what jest does).—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/avajs/ava/issues/1558#issuecomment-369568743, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADWlhVQfoQ2qiR_dK-UIn26li2_F1Wlks5tZ-FSgaJpZM4QAMuE
.
wmertens
on 1 Mar 2018
Does Jest "explain" the - and + in each diff or once per output?
vadimdemedes
on 2 Mar 2018
Git and markdown ```diff ...``` shows new changes with green + and deleted with red -
Jest inverted diff colors is confusing.
01e9
on 29 Mar 2018
@arteniioleg Jest should only be confusing if you assume the colors have the same purpose, they don't.
In Git the colors signifies changes, with green signifying added/new, and red, removed/old. Whereas in Jest the colors signify (the assumed) correctness of the value, with green signifying correct, and red, incorrect.
In both instances the preceding symbol signifies changes, with + signifying added/new, and -, removed/old.
It's also worth noting that Jest goes quite far in explaining (or at least trying to explain) this concept to the user.
nesbocaj
on 29 Mar 2018
Not 100% relating, but I just spent so much time looking into my Unit Tests because I didn't realize what each symbol stood for in this output for a deepEqual:
Difference:
[
- NaN,
- NaN,
+ 12,
+ 30,
]
Why don't we add a simple
- received
+ expected
below? Only the symbols are not completely intuitive to me.
maxrimue
on 26 Feb 2019
Let's start with adding - Received\n+ Expected when we show diffs, and if they're from a snapshot show - Received\n+ Snapshot. We should match the current colors, so the "received" line should be red and the expected/snapshot line green.
In essence, this line should track whether the diff comes from t.snapshot():
https://github.com/avajs/ava/blame/334e15b4af06492c9aed2800a0764f245d6a908b/lib/assert.js#L12
Then the message can be added here:
novemberborn
on 3 Mar 2019
@issuehunt has funded $60.00 to this issue.
- Submit pull request via IssueHunt to receive this reward.
- Want to contribute? Chip in to this issue via IssueHunt.
- Checkout the IssueHunt Issue Explorer to see more funded issues.
- Need help from developers? Add your repository on IssueHunt to raise funds.
IssueHuntBot
on 10 May 2019
I submitted a pull request: #2199
I just modified the concordance opts in the file:
lib\concordance-options.js
Changes the gutters to:
Except I added another commit after the fact to correct my atrocious spelling of the word received.
If this is satisfactory enough, I'd forego that bounty, but I'm not exactly sure how to do that. Just looking to make contributions.
kire73
on 1 Aug 2019
Both due to the age of this issue, and the state of our reporters, I've decided to roll this into #2501.
novemberborn
on 25 May 2020
Related issues
OmgImAlexis
·
3Comments
clitetailor
·
3Comments
avaly
·
4Comments
ivogabe
·
5Comments
ehmicky
·
4Comments