-
Notifications
You must be signed in to change notification settings - Fork 461
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
Add more tests for v flag #4213
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
For full testing of the v
flag it'd be good to make sure that all of the basic tests of the u
flag are also carried out for v
. But it's fine not to consider that in the scope of this PR.
return result ? [result[0], result.index] : null; | ||
} | ||
|
||
assert.sameValue(doExec(/𠮷/).toString(), "𠮷,0", "Basic exec without v flag"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest using assert.compareArray
for these, it's more precise and you'll get better error messages if the test fails.
assert.sameValue(doExec(/𠮷/).toString(), "𠮷,0", "Basic exec without v flag"); | |
assert.compareArray(doExec(/𠮷/).toString(), ["𠮷", 0], "Basic exec without v flag"); |
(etc.)
Same suggestion for elsewhere throughout the PR.
You'll need to add includes: [compareArray.js]
to the front matter for that to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assert.compareArray only works for one-dimensional arrays. For the matchAll test, should I just keep my original string comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not ideal (especially the last assertion with empty matches is kind of confusing), but if you want to leave it as is I'm OK with it.
flatMap
would be nicest, but I'd prefer not to fail the test on engines that haven't implemented flatMap
yet. So that's out.
One possibility could be to do something like this:
function doMatchAll(regex) {
const result = RegExp.prototype[Symbol.matchAll].call(regex, text);
const matches = Array.from(result, m => m[0]);
const indices = Array.from(result, m => m.index);
return matches.concat(indices);
}
// ...
assert.compareArray(
doMatchAll(/𠮷/g),
["𠮷", "𠮷", "𠮷", 0, 3, 6],
"Basic matchAll with g flag"
);
// ...
assert.compareArray(
doMatchAll(/(?:)/gv),
["", "", "", "", "", "", 0, 2, 3, 5, 6, 8],
"Empty matches with v flag"
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I can try this in the follow-up PR. BTW, thanks for the feedback, which is very helpful.
I can open a follow up PR later on for this part. |
a431d6a
to
db13895
Compare
db13895
to
5572fc4
Compare
5572fc4
to
aa0af36
Compare
Fix #4077