test: fix test-cli-node-options on Windows #16709
Conversation
nodejs#16495 broke the Windows build, accounting for `\r\n` newlines should fix it. Ref: nodejs#16495
addaleax
added
cli
|
CI: https://ci.nodejs.org/job/node-test-pull-request/11167/ @nodejs/collaborators @apapirovski This should be fast-tracked to unbreak CI |
| @@ -34,7 +34,7 @@ if (common.hasCrypto) { | |||
| expect('--abort_on-uncaught_exception', 'B\n'); | |||
| expect('--max-old-space-size=0', 'B\n'); | |||
| expect('--stack-trace-limit=100', | |||
| /(\s*at f \(\[eval\]:1:\d*\)\n){100}/, | |||
| /(\s*at f \(\[eval\]:1:\d*\)\r?\n){100}/, | |||
There was a problem hiding this comment.
Maybe worth making it a non-capturing group:
- /(\s*at f \(\[eval\]:1:\d*\)\r?\n){100}/,
+ /(\s*at f \(?:\[eval\]:1:\d*\)\r?\n){100}/, 
There was a problem hiding this comment.
Or rather
- /(\s*at f \(\[eval\]:1:\d*\)\r?\n){100}/,
+ /(?:\s*at f \(\[eval\]:1:\d*\)\r?\n){100}/, There was a problem hiding this comment.
Since this is a test file, I’d go with clarity over performance? I know it took me a while to get the hang of what ?: means when learning regexes and seeing that in the wild…
There was a problem hiding this comment.
Interesting, I find not using non-capturing groups more confusing, as for me a group is for capturing (I think using it for capturing is more common than using it to repeat with (){x}).
The reason I suggested it was because my first thought was "why does the test need to capture 100 different groups", and the fact that the regex gets passed through a custom function makes it harder to track. Performance is a secondary concern.
|
Landed in 3d4d5e0 to unbreak CI Feel free to implement suggestions as separate PRs though :) |
#16495 broke the Windows build, accounting for `\r\n` newlines should fix it. Ref: #16495 PR-URL: #16709 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
nodejs#16495 broke the Windows build, accounting for `\r\n` newlines should fix it. Ref: nodejs#16495 PR-URL: nodejs#16709 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
|
Should land with #16495 when that lands. |
MylesBorins
added
the
baking-for-lts
gibfahn
added
land-on-v8.x
and removed
baking-for-lts
nodejs#16495 broke the Windows build, accounting for `\r\n` newlines should fix it. Ref: nodejs#16495 PR-URL: nodejs#16709 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
#16495 broke the Windows build, accounting for
\r\nnewlines should fix it.Ref: #16495
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
cli