assert.match should return match result

[awwright]

Is your feature request related to a problem? Please describe.
I was using assert(haystack.match(/needle/)); in my unit tests, and I found that assert.match(haystack, /needle/); produced much cleaner errors. +1, suggest to move to stable.

However, in a small handful of cases, I was also using the match results later on in the test, like so:

const m = haystack.match(/nee(\d+)le/);
assert(m);
// ...
assert(haystack2.match(m[1]));

Describe the solution you'd like

If I could rescue the match results from assert.match, and if the regexp argument could be a string, this code could be simplified:

const m = assert.match(haystack, /nee(\d+)le/);
// ...
assert.match(haystack2, m[1]);

Describe alternatives you've considered
This feature simply leads to cleaner tests.

Possible downsides are that people may overlook assert calls used like this, because they always expect "assert" to work like a C assertion, or having a return value is too inconsistent with the rest of the library.

[juanarbol]

Maybe this needs to be discussed, the current implementation is throw if no match. Let me propose the "boolean implementation" so we can start the discussion.

[lundibundi]

I think this can be merged into #33738 if we make it a bit more generic.

[juanarbol]

@lundibundi could you please extend the idea of "generic"?

[lundibundi]

Sorry, I forgot to answer here. My idea was to make all(most) of the methods in assert available via separate API (like mentioned in the PR I linked - isStrictEqual, isDeepStrictEqual etc) so that user will be able to use them without catching the errors and modify the comparison-options (checkPrototype, checkNested, strict/non-strict etc). Also, refer to this comment #33738 (comment) as well.

[BridgeAR]

@awwright thanks for the suggestion. It is mostly untypical that an assertion returns something. I would like to keep the functionality to stick to the principle of least astonishment.

@lundibundi I am not certain if your suggestion would resolve the requested feature.

I am going to close this issue, since there was no further comment for a long time. If someone disagrees, please reopen.

[awwright]

I'm not sure the principle of least astonishment applies, since the proposal is "we should have this feature", not "what is the least astonishing way to implement this feature."

If you're suggesting that my tests should not be astonishing, I think that consideration is outside the scope of Node.js, and without this feature, failed tests are less comprehensible.

[Trott]

I'm not sure the principle of least astonishment applies, since the proposal is "we should have this feature", not "what is the least astonishing way to implement this feature."

If you're suggesting that my tests should not be astonishing, I think that consideration is outside the scope of Node.js, and without this feature, failed tests are less comprehensible.

I think what @BridgeAR was saying was that an assertion returning nothing is the expectation of an assertion and anything else would be surprising. Principle of least astonishment therefore indicates that assertions should not have a return value.

Even if I'm wrong about what @BridgeAR meant, that's certainly how I see it. Conceptually, assertions should not have side effects. In compiled languages, the compilers complain about assertions that have side effects. We don't have that luxury/annoyance in JavaScript, but I do think that conceptually, assertions shouldn't affect the surrounding code. Outside of tests, you should be able to remove assertions and your code should be fine.

That's my opinion, but I'm certainly willing to be persuaded by a convincing use case.

[awwright]

an assertion returning nothing is the expectation of an assertion and anything else would be surprising

Nobody is writing for Node.js with the expectation that the assert library always returns undefined. A return value that I do not ordinarily test for, and don't care about, is not surprising.

Also, Node.js has plenty of functions that have return values where I would not expect them. For example, emitter.setMaxListeners(n) has a return value, which suggests that it is a pure function. A function should mutate state or return a value, a function that does both is surprising.

Conceptually, assertions should not have side effects.

I am not proposing that assertions have side effects. A return value is not a side effect.

If instead I had to write:

assert(m = haystack.match(needle));

This would be a side effect, which is what I'm trying to avoid with this proposal.

[Trott]

If instead I had to write:

assert(m = haystack.match(needle));

This would be a side effect, which is what I'm trying to avoid with this proposal.

I would think the code would be this:

m = haystack.match(needle);
assert(m);

I'm not sure the proposal here improves on that:

m = assert.match(haystack, needle);

As @BridgeAR notes, it seems (to me at least) atypical and therefore surprising that an assertion would return anything. Maybe I need to rethink that, but at first blush, it seems surprising. It would enable patterns I wouldn't want to see like if (assert.match(a, b)).

I suppose it's an easy enough thing to implement if people think it's a good idea. /ping @nodejs/assert Anyone else have additional thoughts?

[ljharb]

I agree that it's not of much value, and I agree that "assertions throw or else don't return anything" is the pretty universal expectation, and it's best not to violate that.

I'm not sure why you can't do assert.match(haystack, needle); const m = haystack.match(needle) if you need both - it's a test, why does avoiding the extra match matter?