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

vsemozhetbyt · 2017-04-15T19:53:53Z

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.

addaleax · 2017-04-15T20:47:37Z

benchmark/common.js

@@ -43,7 +43,7 @@ Benchmark.prototype._parseArgs = function(argv, configs) {
  // Parse configuration arguments
  for (const arg of argv) {
    const match = arg.match(/^(.+?)=([\s\S]*)$/);
-    if (!match || !match[1]) {
+    if (!match || !match[2]) {


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

benjamingr · 2017-04-15T21:02:20Z

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.

vsemozhetbyt · 2017-04-16T01:57:47Z

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

vsemozhetbyt · 2017-04-16T01:59:52Z

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

addaleax · 2017-04-16T20:19:50Z

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>

jasnell · 2017-04-17T21:41:15Z

Landed in e34f8e1

jasnell · 2017-04-17T22:13:20Z

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.

vsemozhetbyt · 2017-04-17T22:18:58Z

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

jasnell · 2017-04-17T22:20:26Z

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.

vsemozhetbyt · 2017-04-17T22:25:13Z

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...

vsemozhetbyt · 2017-04-17T22:51:33Z

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',

vsemozhetbyt · 2017-04-17T23:05:27Z

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

vsemozhetbyt · 2017-04-17T23:51:28Z

CI fails only on Windows possibly with ref to #12475

Dear reviewers, could you re-review?

vsemozhetbyt · 2017-04-17T23:55:03Z

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

jasnell · 2017-04-17T23:55:51Z

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.

vsemozhetbyt · 2017-04-19T04:31:25Z

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

addaleax

Yes, still LGTM :)

benjamingr · 2017-04-19T07:02:38Z

Still LGTM

jseijas · 2017-04-19T07:18:02Z

Still LGTM 👍

jasnell · 2017-04-19T15:14:48Z

Still LGTM

vsemozhetbyt · 2017-04-19T15:32:08Z

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

vsemozhetbyt · 2017-04-19T16:48:24Z

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

vsemozhetbyt · 2017-04-19T23:02:50Z

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>

vsemozhetbyt · 2017-04-20T00:37:11Z

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>

gibfahn · 2017-05-16T11:57:43Z

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 the benchmark subsystem. 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

vsemozhetbyt commented Apr 15, 2017 •

edited

addaleax Apr 15, 2017

benjamingr commented Apr 15, 2017 •

edited

vsemozhetbyt commented Apr 16, 2017

vsemozhetbyt commented Apr 16, 2017

addaleax commented Apr 16, 2017

jasnell commented Apr 17, 2017

jasnell commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

jasnell commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017 •

edited

vsemozhetbyt commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

jasnell commented Apr 17, 2017

vsemozhetbyt commented Apr 19, 2017 •

edited

addaleax left a comment

benjamingr commented Apr 19, 2017

jseijas commented Apr 19, 2017

jasnell commented Apr 19, 2017

vsemozhetbyt commented Apr 19, 2017

vsemozhetbyt commented Apr 19, 2017 •

edited

vsemozhetbyt commented Apr 19, 2017

vsemozhetbyt commented Apr 20, 2017

gibfahn commented May 16, 2017

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

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

Conversation

vsemozhetbyt commented Apr 15, 2017 • edited

Checklist

Affected core subsystem(s)

addaleax Apr 15, 2017

Choose a reason for hiding this comment

benjamingr commented Apr 15, 2017 • edited

vsemozhetbyt commented Apr 16, 2017

vsemozhetbyt commented Apr 16, 2017

addaleax commented Apr 16, 2017

jasnell commented Apr 17, 2017

jasnell commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

jasnell commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017 • edited

vsemozhetbyt commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

vsemozhetbyt commented Apr 17, 2017

jasnell commented Apr 17, 2017

vsemozhetbyt commented Apr 19, 2017 • edited

addaleax left a comment

Choose a reason for hiding this comment

benjamingr commented Apr 19, 2017

jseijas commented Apr 19, 2017

jasnell commented Apr 19, 2017

vsemozhetbyt commented Apr 19, 2017

vsemozhetbyt commented Apr 19, 2017 • edited

vsemozhetbyt commented Apr 19, 2017

vsemozhetbyt commented Apr 20, 2017

gibfahn commented May 16, 2017

vsemozhetbyt commented Apr 15, 2017 •

edited

benjamingr commented Apr 15, 2017 •

edited

vsemozhetbyt commented Apr 17, 2017 •

edited

vsemozhetbyt commented Apr 19, 2017 •

edited

vsemozhetbyt commented Apr 19, 2017 •

edited