benchmark: fix CLI arguments check in common.js by vsemozhetbyt · Pull Request #12429 · nodejs/node · GitHub

Checklist

commit message follows commit guidelines

Affected core subsystem(s)

benchmark

I can't think up any case when match is not null while match[1] is falsy here, as the first group does not use a quantifier that can end up in an empty string. So I suppose the match[2] is intended to prevent arguments like n=. However, I may miss something obvious.

I think you’re right, but it might be even simpler to change * to + in the regex (unless I’m missing something)?

I agree with @addaleax, either we allow passing foo= as a key that's equal an empty value, or we disallow it at which point it's better to match + directly.

@addaleax, @benjamingr PTL, if I've understood you correctly)

BTW, why [\s\S]? Do we have or allow \n in the CLI arguments?

BTW, why [\s\S]? Do we have or allow \n in the CLI arguments?

I would guess it does not actually matter. It might be nice for scripts to be able to set string parameters to whatever they want, including text containing newlines… ¯\_(ツ)_/¯

PR-URL: #12429 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Landed in e34f8e1

Oops, I goofed and landed this without a CI run (it got batched in with a couple of other ones I was working on landing). Looks like the change breaks test/sequential/test-benchmark.http.js. Revert PR is in.

@jasnell Sorry, I had to run the CI. Trying to understand how to fix.

No worries, I should have double checked. I had it grouped in with a set of others that I was landing all together and missed it entirely.

It seems we do have cases as key= as valid CLI params:

node/benchmark/http/check_invalid_header_char.js

Line 9 in 83432bf

'',

Trying to grep another possible cases...

Sorry, I have definitely made a sloppy unchecked assumption about key=:

20 more

node/benchmark/buffers/buffer-tostring.js

Line 6 in 3ee4a1a

encoding: ['', 'utf8', 'ascii', 'latin1', 'binary', 'hex', 'UCS-2'],

node/benchmark/buffers/buffer-write-string.js

Line 6 in 4749ec2

'', 'utf8', 'ascii', 'hex', 'UCS-2', 'utf16le', 'latin1', 'binary'

node/benchmark/buffers/buffer-write-string.js

Line 8 in 4749ec2

args: [ '', 'offset', 'offset+length' ],

node/benchmark/path/basename-posix.js

Line 7 in eba2c62

'',

node/benchmark/path/basename-win32.js

Line 7 in eba2c62

'',

node/benchmark/path/dirname-posix.js

Line 7 in eba2c62

'',

node/benchmark/path/dirname-win32.js

Line 7 in eba2c62

'',

node/benchmark/path/extname-posix.js

Line 7 in eba2c62

'',

https://github.com/nodejs/node/blob/eba2c62bb10fb65785050600212f70d38f1ffba3/benchmark/path/extname-win32.js

node/benchmark/path/isAbsolute-posix.js

Line 7 in eba2c62

'',

node/benchmark/path/isAbsolute-win32.js

Line 7 in eba2c62

'',

node/benchmark/path/join-posix.js

Line 7 in eba2c62

['/foo', 'bar', '', 'baz/asdf', 'quux', '..'].join('|')

node/benchmark/path/join-win32.js

Line 7 in eba2c62

['C:\\foo', 'bar', '', 'baz\\asdf', 'quux', '..'].join('|')

node/benchmark/path/normalize-posix.js

Line 7 in eba2c62

'',

node/benchmark/path/normalize-win32.js

Line 7 in eba2c62

'',

node/benchmark/path/parse-posix.js

Line 7 in eba2c62

'',

node/benchmark/path/parse-win32.js

Line 7 in eba2c62

'',

node/benchmark/path/resolve-posix.js

Line 7 in eba2c62

'',

node/benchmark/path/resolve-win32.js

Line 7 in eba2c62

'',

node/benchmark/util/normalize-encoding.js

Line 14 in 34bf31e

'', 'utf8', 'utf-8', 'UTF-8',

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

CI fails only on Windows possibly with ref to #12475

Dear reviewers, could you re-review?

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

Post mortem on the original botched landing: I believe that I simply had ended up putting this in the wrong list when I was organizing which PRs to land and which needed more work. This one obviously should have gone in the latter list. Since it was a change to benchmarks and we've only recently added benchmark tests to our CI set, I haven't got myself in the habit yet of running make test on benchmark: PRs. I ran make lint and went with it. Sorry all.

@jasnell, @addaleax, @benjamingr, @joyeecheung, @jseijas — Does it still LGTY after the last change?

Yes, still LGTM :)

Still LGTM

Still LGTM 👍

Still LGTM

New CI to be sure: https://ci.nodejs.org/job/node-test-pull-request/7519/

Timeout in sequential/test-benchmark-child-process on Windows. Trying again: https://ci.nodejs.org/job/node-test-pull-request/7521/

UPD. Waiting for #12518

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

PR-URL: #12429 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

CI is OK. Landed in bbbb1f6

PR-URL: #12429 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Marking as dont-land as I think this depends on #7094, which was semver-major. Correct me if I'm wrong.

vsemozhetbyt added the benchmark Issues and PRs related to Node.js benchmarks and benchmarking infrastructure. label Apr 15, 2017

addaleax reviewed Apr 15, 2017

View reviewed changes

addaleax approved these changes Apr 15, 2017

View reviewed changes

benjamingr approved these changes Apr 16, 2017

View reviewed changes

jseijas approved these changes Apr 16, 2017

View reviewed changes

joyeecheung approved these changes Apr 17, 2017

View reviewed changes

jasnell approved these changes Apr 17, 2017

View reviewed changes

jasnell closed this Apr 17, 2017

vsemozhetbyt deleted the bench-args branch April 17, 2017 21:50

jasnell mentioned this pull request Apr 17, 2017

Revert "benchmark: fix CLI arguments check in common.js" #12474

Closed

2 tasks

vsemozhetbyt restored the bench-args branch April 17, 2017 22:13

vsemozhetbyt reopened this Apr 17, 2017

benchmark: fix CLI arguments check in common.js

c6e978e

addaleax approved these changes Apr 19, 2017

View reviewed changes

joyeecheung approved these changes Apr 19, 2017

View reviewed changes

vsemozhetbyt closed this Apr 20, 2017

vsemozhetbyt deleted the bench-args branch April 20, 2017 00:37

evanlucas mentioned this pull request May 1, 2017

v7.10.0 proposal #12775

Merged

gibfahn added the dont-land-on-v6.x label May 16, 2017

benchmark: fix CLI arguments check in common.js by vsemozhetbyt · Pull Request #12429 · nodejs/node · GitHub

Uh oh!

benchmark: fix CLI arguments check in common.js - #12429

benchmark: fix CLI arguments check in common.js#12429vsemozhetbyt wants to merge 1 commit intonodejs:masternodejs/node:masterfrom vsemozhetbyt:bench-argsCopy head branch name to clipboard

Conversation

vsemozhetbyt commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

addaleax Apr 15, 2017

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Apr 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Apr 16, 2017

Uh oh!

vsemozhetbyt commented Apr 16, 2017

Uh oh!

addaleax commented Apr 16, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

vsemozhetbyt commented Apr 17, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

vsemozhetbyt commented Apr 17, 2017

Uh oh!

vsemozhetbyt commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Apr 17, 2017

Uh oh!

vsemozhetbyt commented Apr 17, 2017

Uh oh!

vsemozhetbyt commented Apr 17, 2017

Uh oh!

jasnell commented Apr 17, 2017

Uh oh!

vsemozhetbyt commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Apr 19, 2017

Uh oh!

jseijas commented Apr 19, 2017

Uh oh!

jasnell commented Apr 19, 2017

Uh oh!

vsemozhetbyt commented Apr 19, 2017

Uh oh!

vsemozhetbyt commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Apr 19, 2017

Uh oh!

vsemozhetbyt commented Apr 20, 2017

Uh oh!

gibfahn commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

benchmark: fix CLI arguments check in common.js#12429
vsemozhetbyt wants to merge 1 commit into
nodejs:masterfrom
vsemozhetbyt:bench-args

vsemozhetbyt commented Apr 15, 2017 •
edited
Loading

benjamingr commented Apr 15, 2017 •
edited
Loading

vsemozhetbyt commented Apr 17, 2017 •
edited
Loading

vsemozhetbyt commented Apr 19, 2017 •
edited
Loading

vsemozhetbyt commented Apr 19, 2017 •
edited
Loading