Node: assert diffs are in the wrong direction

[addaleax]

• Version: master
• Platform: any
• Subsystem: assert

> assert.deepStrictEqual('a', 'b') // 'b' is expected
AssertionError [ERR_ASSERTION]: Input A expected to strictly deep-equal input B:
+ expected - actual

- 'a'
+ 'b'

I don’t know whether this is an issue of personal preference or not, but it seems to be pretty odd to me that we’re using expected as the diff target, rather than actual. It’s throwing me off every time I see an assertion, so I’m wondering if other people feel the same way.

(I am not sure, but I think this is because the diff target usually presents the current state of things, rather than what it originally was, i.e. when the test was originally written. But that’s just guessing.)

/cc @BridgeAR

[ryzokuken]

Hmm, it even says + expected - actual. Personal preference, I do think we should go for + actual - expected because it gives the image that "actual" is what we have in place of/replacing "expected".

[targos]

I feel the same way. Also for reference, jest does it the other way:

[ryzokuken]

@BridgeAR @addaleax @targos PTAL at https://github.com/nodejs/node/pull/20929

[BridgeAR]

First: I expected this to come up earlier :laughing:

One thing that is not clear to me is what exactly is "in the wrong direction". This is not a clear term as there are multiple factors to this.

I wrapped my head around this problem for hours already and intensely discussed this with multiple persons and the conclusion was that some parts are likely best to have them the way it is currently implemented while for other aspects there is no ideal solution and something that would satisfy everyone. I checked existing libraries before I implemented it and there is no coherent way of doing this. Yes, jest does it the other way around while other test runners do it the way it is done here and some have a third way of doing this (as in only one aspect might be different but others are the same). I did not take notes when I checked those and I am going to check that again and add this here (please also feel free to add further references so I have to check less ;-) ). So there is definitely no "standard" to it or "best practice" as far as I can tell.
That was a important part for me when implementing it.

One aspect to look at is the color, another is the sign and the last is the comparison order. I looked at all of those individually.

The color green normally represents something "good" while red is "bad". It is relatively safe to say that the good part for assert is the expected value because it is the target and the input diverges from that. Since the "input" diverges, it should be red.

Now looking at the signs. Minus is normally "bad" and plus is "good" (expected). Aligning this with the part above about colors - should be red and + should be green. Having + in red and - in green would also diverge from pretty much any common default towards these two signs. Minus is normally considered as "taking something away" and plus as "something added". If the actual input is bigger, we have to remove something so it fits the expectation. That is the way it is currently implemented.
It could also be argued that - stands for "something missing" and + for "to much". That way the signs should be switched. Even though having a red + and a green - is weird. That clashes in my head.

As Anna pointed out above it can be confusing depending on how you look at things. I believe there is no absolute right or wrong here and I think the main problem is the meaning of the signs each of us has in mind. But the + and - signs are the standard in testing frameworks that I know, so changing that is not really an option.

The comparison order feels right as it is for me and I would not want to change that. The actual value should be on top of the expected even though I do not have strong arguments for it. However, there is one order I would like to change because I think I messed that up: the description order should be actual, expected, skipped and not expected, actual, skipped. This might already be one reason for the confusion. It should definitely be kept aligned with the the comparison order. I already thought about changing that but I was afraid about starting a discussion that would lead to this issue :D

To conclude: I am open to consider changes after carefully weighting the pro and cons. Please be very specific about what exactly throws each of you off (sign, color, order) and tell what should be changed in what way.
If I understand the issue correct we mainly speak about a change to the sign and the color and the order should be kept?

I hope this makes the decision making clearer and highlights that there is no easy solution for this problem.

One more thing: @addaleax gave an example with a primitive. I ask you to also look at objects because that is what I mainly did.

[ryzokuken]

My rationale here for the signs is the part where you point out that "+" signifies something being added, while "-" signified something removed/missing. Because the output bears resemblance to the diff view everyone is familiar with, it's instinctive to see something with a similar pattern here, which is not the case currently.

[addaleax]

The color green normally represents something "good" while red is "bad".

That is dependent on cultural bias – likely not the most qualified person to speak on this, but I don’t think we should be making this kind of assumption.

The comparison order feels right as it is for me and I would not want to change that. The actual value should be on top of the expected even though I do not have strong arguments for it.

I’d disagree. I personally see the actual output as the thing that deviates from the expected output, not the other way around, in part because there is a chronological arrow from “test was written w/ expected output” to “test was run w/ actual output” (and there is a general assumption that a test passes when it is originally written).

the description order should be actual, expected, skipped and not expected, actual, skipped.

:+1:

[BridgeAR]

We changed the order a while ago, so this should be resolved.