test: fix test-cli-node-options on Windows - #16709
Conversation
Sorry, something went wrong.
nodejs#16495 broke the Windows build, accounting for `\r\n` newlines should fix it. Ref: nodejs#16495
|
CI: https://ci.nodejs.org/job/node-test-pull-request/11167/ @nodejs/collaborators @apapirovski This should be fast-tracked to unbreak CI |
Sorry, something went wrong.
| 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}/,
Sorry, something went wrong.
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}/,
Sorry, something went wrong.
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…
Sorry, something went wrong.
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.
Sorry, something went wrong.
|
Landed in 3d4d5e0 to unbreak CI Feel free to implement suggestions as separate PRs though :) |
Sorry, something went wrong.
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 <luigipinca@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
|
Should land with #16495 when that lands. |
Sorry, something went wrong.
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 <luigipinca@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com>
#16495 broke the Windows build, accounting for \r\n newlines should fix it.
Ref: #16495
Checklist
Affected core subsystem(s)
cli