Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert: implement assert.match() #30929

Closed
wants to merge 1 commit into from
Closed

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Dec 13, 2019

This adds a new functionality to the assertion module: a dedicated
check for regular expressions. So far it's possible to use
assert.ok(regexp.test(string)). This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like. It's just known that the
assertion failed.
This allows to pass through the regular expression and the input
string. The string is then matched against the regular expression
and reports a expressive error message in case of a failure.

This is inspired by pull requests that tried to improve our error messages such as #30913.

I checked and we have roughly 1600 assert.ok entries in our tests. From those roughly one third would be a good fit to be refactored to use this pattern instead.
They either use regexp.test(input), input.match(regexp), string.includes(foo), string.startsWith(foo), etc. The error message would be significantly improved over the current one in those cases.

One of the most difficult parts is probably an appropriate name. To keep this open and to potentially still be able to change it, it's marked as experimental.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@BridgeAR BridgeAR requested a review from Trott Dec 13, 2019
@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 13, 2019

To be consistent with assert.equal(actual, exected), I think I'd prefer to have assert.match(str, regexp).

@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Dec 13, 2019

I don’t think this would need to be experimental.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 13, 2019

I don’t think this would need to be experimental.

In prior discussion, Ruben and I both thought experimental made sense because we weren't sure about the method name. test() is the best for being analogous with RegExp.prototype.test() but match() may be more user-friendly, etc. So I proposed putting it in as Experimental and letting some bike-shedding on the name happen.

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 13, 2019

match() is analogous with String.prototype.match() :)

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2019

I am good with either name. If we use test, I would rather stick to the current argument order. If we use match, I would rather switch them.

I'll update it to match, since it sounds like it's the more natural choice for a couple of persons.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 13, 2019

match() is analogous with String.prototype.match() :)

Right. But the biggest difference between match() and test() is that test() returns true or false while match() returns information about the match. To me, this is more closely analogous to test() than match().

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 13, 2019

(To be clear, I'm good with match().)

@targos

This comment has been minimized.

Copy link
Member

targos commented Dec 13, 2019

match() is analogous with String.prototype.match() :)

Right. But the biggest difference between match() and test() is that test() returns true or false while match() returns information about the match. To me, this is more closely analogous to test() than match().

Right. The reason I suggested match is because it makes the order of arguments consistent. Also, if you translate the assertion to English: "assert that the string matches the regexp" sounds better than "assert that the regexp tests the string".

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Dec 13, 2019

match() is analogous with String.prototype.match() :)

Right. But the biggest difference between match() and test() is that test() returns true or false while match() returns information about the match. To me, this is more closely analogous to test() than match().

Right. The reason I suggested match is because it makes the order of arguments consistent. Also, if you translate the assertion to English: "assert that the string matches the regexp" sounds better than "assert that the regexp tests the string".

Yes, I agree. (I certainly hadn't thought about the order of the arguments until you brought it up.)

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2019

I updated the name and the argument order. PTAL.

@targos
targos approved these changes Dec 13, 2019
lib/assert.js Outdated Show resolved Hide resolved
lib/assert.js Show resolved Hide resolved
lib/assert.js Outdated Show resolved Hide resolved
@Trott
Trott approved these changes Dec 13, 2019
@BridgeAR BridgeAR changed the title assert: implement assert.test() assert: implement assert.match() Dec 13, 2019
@nodejs-github-bot

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2019

I just checked other libraries and match seems to be the commonly used one.
While doing that I also noticed that we should actually also implement the negation. The name for that seems not ideal but the most straight forward one would be assert.notMatch(). Any thoughts / suggestions about that?

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2019

I just pushed another commit that adds assert.notMatch(). @targos @Trott PTAL

@nodejs-github-bot

This comment has been minimized.

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 13, 2019

So far it's possible to use assert.ok(regexp.test(string)) This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like.

Can't the second argument of assert.ok() be used to build a user friendly message if desired?

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 13, 2019

@lpinca yes but it requires additional work for the implementer, especially in case it's not a native speaker. Often manual messages actually reduce the amount of information instead of clarifying things.

@lpinca

This comment has been minimized.

Copy link
Member

lpinca commented Dec 13, 2019

I'm not sure if a default generated user friendly error message (is it really always needed?) justifies the addition of a new method.

lib/assert.js Show resolved Hide resolved
* `regexp` {RegExp}
* `message` {string|Error}

> Stability: 1 - Experimental

This comment has been minimized.

Copy link
@jasnell

jasnell Dec 15, 2019

Member

Not really convinced this needs to be experimental

This comment has been minimized.

Copy link
@Trott

Trott Dec 15, 2019

Member

Not really convinced this needs to be experimental

It may not need to be experimental, true, but at the same time, it does no harm and it allows us to take a few releases before committing to the API particulars (the method's name, the order of the arguments, whether or not to have a corresponding not method, etc.).

To be honest, I'd be a little more reluctant to approve this if it wasn't Experimental. Once it goes in as Stable, we will never be able to remove it. And while we're not there yet, I do worry about the assert API eventually becoming a million functions no one can remember instead of a small, clear, manageable API surface area.

@nodejs-github-bot

This comment has been minimized.

lib/assert.js Outdated Show resolved Hide resolved
@Trott Trott added the experimental label Dec 22, 2019
@Qard
Qard approved these changes Dec 23, 2019
Copy link
Member

Qard left a comment

LGTM. I'm indifferent about notMatch versus doesNotMatch and about if it should be marked as experimental. Whichever is fine with me. I think the feature itself looks good though, and should help to improve test reporting in the future. :)

@nodejs-github-bot

This comment was marked as outdated.

@BridgeAR BridgeAR force-pushed the BridgeAR:add-assert-test branch from 2ae95f7 to 2300750 Dec 24, 2019
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 25, 2019

I marked this as author ready, since this got multiple LGs and the recent name change got thumbs up as well. Please remove the label in case someone disagrees.

This adds a new functionality to the assertion module: a dedicated
check for regular expressions. So far it's possible to use
`assert.ok(regexp.test(string))`. This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like. It's just known that the
assertion failed.
This allows to pass through the regular expression and the input
string. The string is then matched against the regular expression
and reports a expressive error message in case of a failure.
@BridgeAR BridgeAR force-pushed the BridgeAR:add-assert-test branch from 2300750 to 4f82ee6 Dec 31, 2019
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Dec 31, 2019

Rebased due to conflicts.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

BridgeAR added a commit that referenced this pull request Jan 1, 2020
This adds a new functionality to the assertion module: a dedicated
check for regular expressions. So far it's possible to use
`assert.ok(regexp.test(string))`. This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like. It's just known that the
assertion failed.
This allows to pass through the regular expression and the input
string. The string is then matched against the regular expression
and reports a expressive error message in case of a failure.

PR-URL: #30929
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Jan 1, 2020

Landed in e33f773 🎉

@BridgeAR BridgeAR closed this Jan 1, 2020
BridgeAR added a commit that referenced this pull request Jan 3, 2020
This adds a new functionality to the assertion module: a dedicated
check for regular expressions. So far it's possible to use
`assert.ok(regexp.test(string))`. This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like. It's just known that the
assertion failed.
This allows to pass through the regular expression and the input
string. The string is then matched against the regular expression
and reports a expressive error message in case of a failure.

PR-URL: #30929
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
BridgeAR added a commit that referenced this pull request Jan 7, 2020
Notable changes:

* assert:
  * Implement `assert.match()` and `assert.doesNotMatch()` (Ruben
    Bridgewater) #30929
* events:
  * Add `EventEmitter.on` to async iterate over events (Matteo Collina)
    #27994
  * Allow monitoring error events (Gerhard Stoebich)
    #30932
* fs:
  * Allow overriding `fs` for streams (Robert Nagy)
    #29083
* perf_hooks:
  * Move `perf_hooks` out of experimental (legendecas)
    #31101
* repl:
  * Implement ZSH-like reverse-i-search (Ruben Bridgewater)
    #31006
* tls:
  * Add PSK (pre-shared key) support (Denys Otrishko)
    #23188

PR-URL: #31238
ljharb added a commit to ljharb/tape that referenced this pull request Jan 8, 2020
ljharb added a commit to ljharb/tape that referenced this pull request Jan 8, 2020
@targos

This comment has been minimized.

Copy link
Member

targos commented Jan 14, 2020

This needs a backport or other previous PRs to be backported in order to land on v12.x-staging.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 20, 2020
This adds a new functionality to the assertion module: a dedicated
check for regular expressions. So far it's possible to use
`assert.ok(regexp.test(string))`. This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like. It's just known that the
assertion failed.
This allows to pass through the regular expression and the input
string. The string is then matched against the regular expression
and reports a expressive error message in case of a failure.

PR-URL: nodejs#30929
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
BridgeAR added a commit to BridgeAR/node that referenced this pull request Jan 21, 2020
This adds a new functionality to the assertion module: a dedicated
check for regular expressions. So far it's possible to use
`assert.ok(regexp.test(string))`. This is not ideal though when it
comes to the error message, since it's not possible to know how
either of the input values look like. It's just known that the
assertion failed.
This allows to pass through the regular expression and the input
string. The string is then matched against the regular expression
and reports a expressive error message in case of a failure.

PR-URL: nodejs#30929
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.