assert: implement assert.match() #30929

BridgeAR · 2019-12-13T03:20:23Z

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

targos · 2019-12-13T08:14:46Z

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

addaleax · 2019-12-13T15:22:24Z

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

Trott · 2019-12-13T15:55:27Z

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 · 2019-12-13T15:58:56Z

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

BridgeAR · 2019-12-13T16:01:48Z

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 · 2019-12-13T16:03:48Z

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 · 2019-12-13T16:04:21Z

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

targos · 2019-12-13T16:06:59Z

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 · 2019-12-13T16:08:55Z

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 · 2019-12-13T16:30:21Z

I updated the name and the argument order. PTAL.

lib/assert.js

nodejs-github-bot · 2019-12-13T18:10:28Z

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

BridgeAR · 2019-12-13T18:23:57Z

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 · 2019-12-13T20:04:50Z

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

nodejs-github-bot · 2019-12-13T20:05:54Z

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

lpinca · 2019-12-13T21:12:47Z

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 · 2019-12-13T21:17:18Z

@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 · 2019-12-13T21:22:37Z

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

jasnell · 2019-12-15T20:36:02Z

doc/api/assert.md

+* `regexp` {RegExp}
+* `message` {string|Error}
+
+> Stability: 1 - Experimental


Not really convinced this needs to be experimental

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 · 2019-12-19T10:15:37Z

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

lib/assert.js

Qard

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 · 2019-12-31T15:12:58Z

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

nodejs-github-bot · 2020-01-01T11:54:15Z

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

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 · 2020-01-01T17:46:12Z

Landed in e33f773 🎉

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>

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

targos · 2020-01-14T13:12:47Z

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

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>

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. Backport-PR-URL: #31431 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>

Notable changes: New assert APIs The `assert` module now provides experimental `assert.match()` and `assert.doesNotMatch()` methods. They will validate that the first argument is a string and matches (or does not match) the provided regular expression This is an experimental feature. Ruben Bridgewater [#30929](#30929). Advanced serialization for IPC The `child_process` and `cluster` modules now support a `serialization` option to change the serialization mechanism used for IPC. The option can have one of two values: * `'json'` (default): `JSON.stringify()` and `JSON.parse()` are used. This is how message serialization was done before. * `'advanced'`: The serialization API of the `v8` module is used. It is based on the HTML structured clone algorithm. and is able to serialize more built-in JavaScript object types, such as `BigInt`, `Map`, `Set` etc. as well as circular data structures. Anna Henningsen [#30162](#30162). CLI flags The new `--trace-exit` CLI flag makes Node.js print a stack trace whenever the Node.js environment is exited proactively (i.e. by invoking the `process.exit()` function or pressing Ctrl+C). legendecas [#30516](#30516). ___ The new `--trace-uncaught` CLI flag makes Node.js print a stack trace at the time of throwing uncaught exceptions, rather than at the creation of the `Error` object, if there is any. This option is not enabled by default because it may affect garbage collection behavior negatively. Anna Henningsen [#30025](#30025). ___ The `--disallow-code-generation-from-strings` V8 CLI flag is now whitelisted in the `NODE_OPTIONS` environment variable. Shelley Vohr [#30094](#30094). New crypto APIs For DSA and ECDSA, a new signature encoding is now supported in addition to the existing one (DER). The `verify` and `sign` methods accept a `dsaEncoding` option, which can have one of two values: * `'der'` (default): DER-encoded ASN.1 signature structure encoding `(r, s)`. * `'ieee-p1363'`: Signature format `r || s` as proposed in IEEE-P1363. Tobias Nießen [#29292](#29292). ___ A new method was added to `Hash`: `Hash.prototype.copy`. It makes it possible to clone the internal state of a `Hash` object into a new `Hash` object, allowing to compute the digest between updates. Ben Noordhuis [#29910](#29910). Dependency updates libuv was updated to 1.34.0. This includes fixes to `uv_fs_copyfile()` and `uv_interface_addresses()` and adds two new functions: `uv_sleep()` and `uv_fs_mkstemp()`. Colin Ihrig [#30783](#30783). ___ V8 was updated to 7.8.279.23. This includes performance improvements to object destructuring, RegExp match failures and WebAssembly startup time. The official release notes are available at https://v8.dev/blog/v8-release-78. Michaël Zasso [#30109](#30109). New EventEmitter APIs The new `EventEmitter.on` static method allows to async iterate over events. Matteo Collina [#27994](#27994). ___ It is now possible to monitor `'error'` events on an `EventEmitter` without consuming the emitted error by installing a listener using the symbol `EventEmitter.errorMonitor`. Gerhard Stoebich [#30932](#30932). ___ Using `async` functions with event handlers is problematic, because it can lead to an unhandled rejection in case of a thrown exception. The experimental `captureRejections` option in the `EventEmitter` constructor or the global setting change this behavior, installing a `.then(undefined, handler)` handler on the `Promise`. This handler routes the exception asynchronously to the `Symbol.for('nodejs.rejection')` method if there is one, or to the `'error'` event handler if there is none. Setting `EventEmitter.captureRejections = true` will change the default for all new instances of `EventEmitter`. This is an experimental feature. Matteo Collina [#27867](#27867). Performance Hooks are no longer experimental The `perf_hooks` module is now considered a stable API. legendecas [#31101](#31101). Introduction of experimental WebAssembly System Interface (WASI) support A new core module, `wasi`, is introduced to provide an implementation of the [WebAssembly System Interface](https://wasi.dev/) specification. WASI gives sandboxed WebAssembly applications access to the underlying operating system via a collection of POSIX-like functions. This is an experimental feature. Colin Ihrig [#30258](#30258). PR-URL: #31691

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 13, 2019

BridgeAR requested a review from Trott December 13, 2019 03:20

nodejs-github-bot added the assert Issues and PRs related to the assert subsystem. label Dec 13, 2019

targos approved these changes Dec 13, 2019

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

lib/assert.js Show resolved Hide resolved

lib/assert.js Outdated Show resolved Hide resolved

Trott approved these changes Dec 13, 2019

View reviewed changes

BridgeAR changed the title ~~assert: implement assert.test()~~ assert: implement assert.match() Dec 13, 2019

legendecas reviewed Dec 15, 2019

View reviewed changes

lib/assert.js Show resolved Hide resolved

jasnell reviewed Dec 15, 2019

View reviewed changes

BridgeAR commented Dec 20, 2019

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

Trott added the experimental Issues and PRs related to experimental features. label Dec 22, 2019

Qard approved these changes Dec 23, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

BridgeAR closed this Jan 1, 2020

BridgeAR mentioned this pull request Jan 7, 2020

v13.6.0 proposal #31238

Merged

targos added the backport-requested-v12.x label Jan 14, 2020

BridgeAR mentioned this pull request Jan 22, 2020

[v12.x] multiple util and assert backports #31431

Closed

4 tasks

BridgeAR added the backport-open-v12.x label Jan 22, 2020

MylesBorins added backported-to-v12.x and removed backport-open-v12.x labels Jan 30, 2020

targos mentioned this pull request Jan 30, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

assert: implement assert.match() #30929

assert: implement assert.match() #30929

BridgeAR commented Dec 13, 2019 •

edited

Loading

targos commented Dec 13, 2019

addaleax commented Dec 13, 2019

Trott commented Dec 13, 2019

targos commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

Trott commented Dec 13, 2019

Trott commented Dec 13, 2019

targos commented Dec 13, 2019

Trott commented Dec 13, 2019 •

edited

Loading

BridgeAR commented Dec 13, 2019

nodejs-github-bot commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

nodejs-github-bot commented Dec 13, 2019

lpinca commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

lpinca commented Dec 13, 2019 •

edited

Loading

jasnell Dec 15, 2019

Trott Dec 15, 2019 •

edited

Loading

nodejs-github-bot commented Dec 19, 2019

Qard left a comment

This comment has been minimized.

nodejs-github-bot commented Dec 31, 2019

nodejs-github-bot commented Jan 1, 2020

BridgeAR commented Jan 1, 2020

targos commented Jan 14, 2020

assert: implement assert.match() #30929

assert: implement assert.match() #30929

Conversation

BridgeAR commented Dec 13, 2019 • edited Loading

Checklist

targos commented Dec 13, 2019

addaleax commented Dec 13, 2019

Trott commented Dec 13, 2019

targos commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

Trott commented Dec 13, 2019

Trott commented Dec 13, 2019

targos commented Dec 13, 2019

Trott commented Dec 13, 2019 • edited Loading

BridgeAR commented Dec 13, 2019

nodejs-github-bot commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

nodejs-github-bot commented Dec 13, 2019

lpinca commented Dec 13, 2019

BridgeAR commented Dec 13, 2019

lpinca commented Dec 13, 2019 • edited Loading

jasnell Dec 15, 2019

Choose a reason for hiding this comment

Trott Dec 15, 2019 • edited Loading

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 19, 2019

Qard left a comment

Choose a reason for hiding this comment

This comment has been minimized.

nodejs-github-bot commented Dec 31, 2019

nodejs-github-bot commented Jan 1, 2020

BridgeAR commented Jan 1, 2020

targos commented Jan 14, 2020

BridgeAR commented Dec 13, 2019 •

edited

Loading

Trott commented Dec 13, 2019 •

edited

Loading

lpinca commented Dec 13, 2019 •

edited

Loading

Trott Dec 15, 2019 •

edited

Loading