assert: allow arbitrary argument order in match() and doesNotMatch()

[BridgeAR]

This allows assert.match() and assert.doesNotMatch() arguments to
be inserted in any order.

Signed-off-by: Ruben Bridgewater [email protected]

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/41176/

[Trott]

I'm -0.01 on this, so definitely not blocking. But I prefer one correct way over this kind of "do whatever you like" kind of thing. Moreover, the current signature nicely matches the way assert.strictEqual() and others expect to take the actual value first and the expected value second. Lastly, from a "what would TC39 do?" perspective, I feel like they would mandate the correct order.

[BridgeAR]

@Trott I personally very well see the point that a single argument order is mostly ideal. I share that view in general. However, some APIs make it more difficult to remember the correct argument order. This is "easy to detect" due to failing if it's the wrong order and as such, I am also fine with it. Yet, it would be more user friendly to accept both ways. That way there is no need to change the implementation and developers require a tad less time using the feature.
This is only possible to do in cases where we are guaranteed to understand the user intend both ways and that is AFAIC the case. That's why I suggest this change.

[benjamingr]

I agree with @Trott in general too it's just in this particular API I ran into this a few times and it would have been convenient if it "just worked" in the other order too.

[tniessen]

+1 to what @Trott said, mostly because someone might be using this function to verify that the types are as expected.

const [shouldBeString, shouldBeRegExp] = blackboxFunction();
assert.match(shouldBeString, shouldBeRegexp);
// Unexpectedly passes if the function returns the values in the wrong order.

[BridgeAR]

@tniessen I would expect people would explicitly check the variables for their correct type in such case. Otherwise it's something that works "by chance", since it's just an implementation detail that the arguments would throw an ERR_INVALID_ARG_TYPE error (not an AssertionError) in that case. Don't you think so?

[tniessen]

@tniessen I would expect people would explicitly check the variables for their correct type in such case. Otherwise it's something that works "by chance", since it's just an implementation detail that the arguments would throw an ERR_INVALID_ARG_TYPE error (not an AssertionError) in that case. Don't you think so?

I disagree. The current behavior doesn't work "by chance," it is guaranteed as per the documentation:

If the values do match, or if the `string` argument is of another type than
`string`, an [`AssertionError`][] is thrown

If the values do not match, or if the `string` argument is of another type than
`string`, an [`AssertionError`][] is thrown

[BridgeAR]

@tniessen this only applies to the string argument. Switching the argument order would cause the following error at the moment:

You would loose the information about the other argument completely.

[tniessen]

@BridgeAR Still, the implication of the documented behavior is that there is a guarantee that an error will be thrown if the first two arguments are not (string: string, pattern: RegExp). This change removes that implication and makes the guarantee weaker.

We should be extremely careful when we intentionally weaken assertions.

[Trott]

If we do go ahead with this, I wonder if it might make sense to emit a warning.

[tniessen]

Formally, I would say that an assertion works if it rejects any input that does not belong to a well-defined set of acceptable inputs. Currently, the set of acceptable inputs is a strict subset of string × RegExp, and this is documented and guaranteed behavior. Thus, the assertion works as expected only if it rejects all inputs that are not in string × RegExp. In particular, the assertion works as expected only if it rejects all inputs in RegExp × string.

With this change, the set of acceptable inputs grows, effectively weakening the assertion. By the above definition, the new implementation does not work, at least not as stated by the current documentation.

Tests, and assertions in particular, are vital to software quality. Ensuring that the arguments are exactly as expected is the whole purpose of these assertions.