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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Apr 15, 2017

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

@vsemozhetbyt vsemozhetbyt added the benchmark Issues and PRs related to the benchmark subsystem. label Apr 15, 2017
@@ -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]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@benjamingr
Copy link
Member

benjamingr commented Apr 15, 2017

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
Copy link
Contributor Author

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

@vsemozhetbyt
Copy link
Contributor Author

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

@addaleax
Copy link
Member

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… ¯\_(ツ)_/¯

jasnell pushed a commit that referenced this pull request Apr 17, 2017
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
Copy link
Member

jasnell commented Apr 17, 2017

Landed in e34f8e1

@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

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 vsemozhetbyt restored the bench-args branch April 17, 2017 22:13
@vsemozhetbyt vsemozhetbyt reopened this Apr 17, 2017
@vsemozhetbyt
Copy link
Contributor Author

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

@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

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
Copy link
Contributor Author

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

Trying to grep another possible cases...

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 17, 2017

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

20 more

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

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

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






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


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

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







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

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

CI fails only on Windows possibly with ref to #12475

Dear reviewers, could you re-review?

@vsemozhetbyt
Copy link
Contributor Author

@jasnell
Copy link
Member

jasnell commented Apr 17, 2017

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
Copy link
Contributor Author

vsemozhetbyt commented Apr 19, 2017

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

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, still LGTM :)

@benjamingr
Copy link
Member

Still LGTM

@jseijas
Copy link

jseijas commented Apr 19, 2017

Still LGTM 👍

@jasnell
Copy link
Member

jasnell commented Apr 19, 2017

Still LGTM

@vsemozhetbyt
Copy link
Contributor Author

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Apr 19, 2017

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
Copy link
Contributor Author

vsemozhetbyt added a commit that referenced this pull request Apr 20, 2017
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
Copy link
Contributor Author

CI is OK. Landed in bbbb1f6

@vsemozhetbyt vsemozhetbyt deleted the bench-args branch April 20, 2017 00:37
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
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>
evanlucas pushed a commit that referenced this pull request Apr 25, 2017
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>
@evanlucas evanlucas mentioned this pull request May 1, 2017
evanlucas pushed a commit that referenced this pull request May 1, 2017
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>
evanlucas pushed a commit that referenced this pull request May 1, 2017
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>
evanlucas pushed a commit that referenced this pull request May 2, 2017
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>
evanlucas pushed a commit that referenced this pull request May 2, 2017
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
Copy link
Member

gibfahn commented May 16, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants