benchmark: fix CLI arguments check in common.js #12429
Conversation
vsemozhetbyt
added
the
benchmark
benchmark/common.js
Outdated
| @@ -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]) { | |||
There was a problem hiding this comment.
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 |
|
@addaleax, @benjamingr PTL, if I've understood you correctly) |
|
BTW, why |
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
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 Trying to grep another possible cases... |
|
Sorry, I have definitely made a sloppy unchecked assumption about 20 morenode/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/join-posix.js Line 7 in eba2c62 node/benchmark/path/join-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 |
|
CI fails only on Windows possibly with ref to #12475 Dear reviewers, could you re-review? |
|
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 |
|
@jasnell, @addaleax, @benjamingr, @joyeecheung, @jseijas — Does it still LGTY after the last change? |
|
Still LGTM |
|
Still LGTM 👍 |
|
Still LGTM |
|
New CI to be sure: https://ci.nodejs.org/job/node-test-pull-request/7519/ |
|
Timeout in UPD. Waiting for #12518 |
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
CI is OK. Landed in bbbb1f6 |
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #12429 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Marking as |
Checklist
Affected core subsystem(s)
benchmark
I can't think up any case when
matchis notnullwhilematch[1]is falsy here, as the first group does not use a quantifier that can end up in an empty string.So I suppose theHowever, I may miss something obvious.match[2]is intended to prevent arguments liken=.