Explain diff gutter symbols

[wmertens]

Please see this comment: #1558 (comment)

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

ava/lib/assert.js

Line 321 in 14f7095

values: [formatDescriptorDiff(result.actual, result.expected, {invert: true})]

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

[novemberborn]

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 for that.

If that were fixed would you be happy with the current behavior?

[wmertens]

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 ***@***.***> 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 <concordancejs/concordance#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 <#1558 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlqOsoB5v_2zAw7RumpYqfpqOobomks5svKe0gaJpZM4QAMuE> .

[novemberborn]

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.

[nesbocaj]

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.

[novemberborn]

@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?

[nesbocaj]

@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]

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

[novemberborn]

@nesbocaj makes sense. @wmertens what do you think?

[rzec-r7]

@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).

[wmertens]

Yes, the way Jest does it looks good!

…

On Thu, Mar 1, 2018 at 12:53 PM rzec-r7 ***@***.***> 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 <#1558 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADWlhVQfoQ2qiR_dK-UIn26li2_F1Wlks5tZ-FSgaJpZM4QAMuE> .

[vadimdemedes]

Does Jest "explain" the - and + in each diff or once per output?

[01e9]

Git and markdown ```diff ...``` shows new changes with green + and deleted with red -
Jest inverted diff colors is confusing.

[nesbocaj]

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

[maxrimue]

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.

[novemberborn]

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:

https://github.com/avajs/ava/blame/334e15b4af06492c9aed2800a0764f245d6a908b/lib/reporters/format-serialized-error.js#L16

[IssueHuntBot]

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

[kire73]

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.

[novemberborn]

Both due to the age of this issue, and the state of our reporters, I've decided to roll this into #2501.