test: fix RegExp nits #13770

vsemozhetbyt · 2017-06-18T23:06:05Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test

It will be easier to review this PR commit by commit, performance-wise and for better comprehending (different changes are confusingly interwoven in the overall diff).

Remove redundant RegExp part.

RegExp part with a quantifier '0 or more times' is redundant if it is used at the very edge of the RegExp and is not captured or does not affect match indices (UPD: and should not greedily take up previous similar parts).
Remove needless RegExp flag.

In fixed case, /g flag is needless in the boolean context.
Remove needless RegExp capturing.

Use non-capturing grouping or remove capturing completely when:
- capturing is useless per se, e.g. in test() check;
- captured groups are not used afterward at all;
- some of the later captured groups are not used afterward.
Use test(), not match() or exec() in boolean context.

match() and exec() return a complicated object, unneeded in a boolean context. This fix also makes the code more clear and predictable.
Do not needlessly repeat RegExp creation.

This is the largest commit, however, it is rather easy to skim. It takes RegExp creation out of cycles and other repetitions.

As long as the RegExp does not use /g flag + match indices, we are safe here.

In tests, this fix hardly gives a significant performance gain, but it increases clarity and maintainability, reassuring some RegExps to be identical.

RegExps in functions are not taken out of their functions: while these functions are called many times and their RegExps are recreated with each call, the performance gain in test cases does not seem to be worth decreasing function self-dependency.

RegExp part with a quantifier '0 or more' is redundant if it is used at the very edge of the RegExp and is not captured or does not affect match indices.

In fixed case, `/g` flag is needless in the boolean context.

Use non-capturing grouping or remove capturing completely when: * capturing is useless per se, e.g. in test() check; * captured groups are not used afterward at all; * some of the later captured groups are not used afterward.

match() and exec() return a complicated object, unneeded in a boolean context.

This commit takes RegExp creation out of cycles and other repetitions. As long as the RegExp does not use /g flag and match indices, we are safe here. In tests, this fix hardly gives a significant performance gain, but it increases clarity and maintainability, reassuring some RegExps to be identical. RegExp in functions are not taken out of their functions: while these functions are called many times and their RegExps are recreated with each call, the performance gain in test cases does not seem to be worth decreasing function self-dependency.

vsemozhetbyt · 2017-06-18T23:07:48Z

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

cjihrig

LGTM if the CI passes (it's looking good so far).

not-an-aardvark · 2017-06-19T06:23:31Z

test/inspector/test-inspector-ip-detection.js

@@ -22,7 +22,7 @@ function checkListResponse(instance, err, response) {
  const match = wsUrl.match(/^ws:\/\/(.*):9229\/(.*)/);
  assert.strictEqual(ip, match[1]);
  assert.strictEqual(res['id'], match[2]);
-  assert.strictEqual(ip, res['devtoolsFrontendUrl'].match(/.*ws=(.*):9229/)[1]);
+  assert.strictEqual(ip, res['devtoolsFrontendUrl'].match(/ws=(.*):9229/)[1]);


This change actually can have an effect on the match returned:

const string = 'ws=0.0.0.0:9229 ws=1.1.1.1:9229'; string.match(/.*ws=(.*):9229/)[1] // => '1.1.1.1' string.match(/ws=(.*):9229/)[1] // => '0.0.0.0:9229 ws=1.1.1.1'

I'm not sure whether this matters for this particular test case.

[EDITED by @Trott to correct small typo in code: 9299 -> 9229]

Oh, sorry, I've missed this part can take up similar parts.

However, this RegExp checks a URL like:
chrome-devtools://devtools/bundled/inspector.html?experiments=true&v8only=true&ws=192.168.137.2:9229/151fc61d-6045-4692-b651-58bb5ba2c83d

If there is any chance this URL can have two ws= parts, let me know and I will remove this commit.

cc @nodejs/v8-inspector

I would leave it as is. If there's no good reason to make the test more lax, don't make it more lax. :-D

If the intention is to introduce a lint rule that would flag something like this line, it can always be disabled on just this line with a comment.

* Remove needless RegExp flag In fixed case, `/g` flag is needless in the boolean context. * Remove needless RegExp capturing Use non-capturing grouping or remove capturing completely when: * capturing is useless per se, e.g. in test() check; * captured groups are not used afterward at all; * some of the later captured groups are not used afterward. * Use test, not match/exec in boolean context match() and exec() return a complicated object, unneeded in a boolean context. * Do not needlessly repeat RegExp creation This commit takes RegExp creation out of cycles and other repetitions. As long as the RegExp does not use /g flag and match indices, we are safe here. In tests, this fix hardly gives a significant performance gain, but it increases clarity and maintainability, reassuring some RegExps to be identical. RegExp in functions are not taken out of their functions: while these functions are called many times and their RegExps are recreated with each call, the performance gain in test cases does not seem to be worth decreasing function self-dependency. PR-URL: #13770 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vsemozhetbyt · 2017-06-21T00:50:35Z

Landed in 76340e3 (the first commit was dropped according to the request).

* Remove needless RegExp flag In fixed case, `/g` flag is needless in the boolean context. * Remove needless RegExp capturing Use non-capturing grouping or remove capturing completely when: * capturing is useless per se, e.g. in test() check; * captured groups are not used afterward at all; * some of the later captured groups are not used afterward. * Use test, not match/exec in boolean context match() and exec() return a complicated object, unneeded in a boolean context. * Do not needlessly repeat RegExp creation This commit takes RegExp creation out of cycles and other repetitions. As long as the RegExp does not use /g flag and match indices, we are safe here. In tests, this fix hardly gives a significant performance gain, but it increases clarity and maintainability, reassuring some RegExps to be identical. RegExp in functions are not taken out of their functions: while these functions are called many times and their RegExps are recreated with each call, the performance gain in test cases does not seem to be worth decreasing function self-dependency. PR-URL: #13770 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins · 2017-07-17T18:43:46Z

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

vsemozhetbyt · 2017-07-19T16:43:49Z

@MylesBorins Backport: #14370

* Remove needless RegExp flag In fixed case, `/g` flag is needless in the boolean context. * Remove needless RegExp capturing Use non-capturing grouping or remove capturing completely when: * capturing is useless per se, e.g. in test() check; * captured groups are not used afterward at all; * some of the later captured groups are not used afterward. * Use test, not match/exec in boolean context match() and exec() return a complicated object, unneeded in a boolean context. * Do not needlessly repeat RegExp creation This commit takes RegExp creation out of cycles and other repetitions. As long as the RegExp does not use /g flag and match indices, we are safe here. In tests, this fix hardly gives a significant performance gain, but it increases clarity and maintainability, reassuring some RegExps to be identical. RegExp in functions are not taken out of their functions: while these functions are called many times and their RegExps are recreated with each call, the performance gain in test cases does not seem to be worth decreasing function self-dependency. Backport-PR-URL: #14370 PR-URL: #13770 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

vsemozhetbyt added 5 commits June 18, 2017 16:04

test: remove redundant RegExp part

9737204

RegExp part with a quantifier '0 or more' is redundant if it is used at the very edge of the RegExp and is not captured or does not affect match indices.

test: remove needless RegExp flag

035937b

In fixed case, `/g` flag is needless in the boolean context.

test: remove needless RegExp capturing

aa006ff

Use non-capturing grouping or remove capturing completely when: * capturing is useless per se, e.g. in test() check; * captured groups are not used afterward at all; * some of the later captured groups are not used afterward.

test: use test, not match/exec in boolean context

dc0d79c

match() and exec() return a complicated object, unneeded in a boolean context.

nodejs-github-bot added debugger doc Issues and PRs related to the documentations. inspector Issues and PRs related to the V8 inspector protocol test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 18, 2017

vsemozhetbyt removed debugger doc Issues and PRs related to the documentations. inspector Issues and PRs related to the V8 inspector protocol tools Issues and PRs related to the tools directory. labels Jun 18, 2017

cjihrig approved these changes Jun 18, 2017

View reviewed changes

not-an-aardvark reviewed Jun 19, 2017

View reviewed changes

vsemozhetbyt closed this Jun 21, 2017

vsemozhetbyt deleted the test-regexp-nits branch June 21, 2017 00:50

addaleax mentioned this pull request Jun 21, 2017

v8.2.0 proposal #13744

Merged

addaleax mentioned this pull request Jun 21, 2017

v8.1.3 proposal #13861

Merged

MylesBorins added the backport-requested-v6.x label Jul 17, 2017

vsemozhetbyt mentioned this pull request Jul 19, 2017

[v6.x backport] test: fix RegExp nits #14370

Closed

2 tasks

MylesBorins added backported-to-v6.x and removed backport-requested-v6.x labels Jul 21, 2017

MylesBorins mentioned this pull request Jul 21, 2017

v6.11.2 proposal #14356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: fix RegExp nits #13770

test: fix RegExp nits #13770

vsemozhetbyt commented Jun 18, 2017 •

edited

Loading

vsemozhetbyt commented Jun 18, 2017

cjihrig left a comment

not-an-aardvark Jun 19, 2017 •

edited by Trott

Loading

vsemozhetbyt Jun 19, 2017

Trott Jun 19, 2017

vsemozhetbyt commented Jun 21, 2017 •

edited

Loading

MylesBorins commented Jul 17, 2017

vsemozhetbyt commented Jul 19, 2017

test: fix RegExp nits #13770

test: fix RegExp nits #13770

Conversation

vsemozhetbyt commented Jun 18, 2017 • edited Loading

Checklist

Affected core subsystem(s)

vsemozhetbyt commented Jun 18, 2017

cjihrig left a comment

Choose a reason for hiding this comment

not-an-aardvark Jun 19, 2017 • edited by Trott Loading

Choose a reason for hiding this comment

vsemozhetbyt Jun 19, 2017

Choose a reason for hiding this comment

Trott Jun 19, 2017

Choose a reason for hiding this comment

vsemozhetbyt commented Jun 21, 2017 • edited Loading

MylesBorins commented Jul 17, 2017

vsemozhetbyt commented Jul 19, 2017

vsemozhetbyt commented Jun 18, 2017 •

edited

Loading

not-an-aardvark Jun 19, 2017 •

edited by Trott

Loading

vsemozhetbyt commented Jun 21, 2017 •

edited

Loading