Add basic u flag tests in v flag tests. #4241
Conversation
| undefined, | ||
| "Non-ASCII with v flag" | ||
| ); | ||
| reportCompare(0, 0); |
There was a problem hiding this comment.
| reportCompare(0, 0); | |
| reportCompare(0, 0); | |
The lint test seems to be failing due to this missing newline in the end of the file.
There was a problem hiding this comment.
Thanks for the guidance.
|
@tannal thanks for the followup PR! I think it would be better to add the u flag tests to separate files, or alternatively to rename the test files to mention the u flag as well, e.g., |
tannal
force-pushed
the
regex-v-flag-follow-up
branch
from
0b1f976
to
5c20b3b
Compare
There was a problem hiding this comment.
I made some suggestions to have the tests passing, however, there are two tests in test/built-ins/String/prototype/matchAll/regexp-prototype-matchAll-v-u-flag.js that fail unexpectedly. We could remove them for now but I would prefer if we knew why they are failing and how to fix them.
| return result ? [result[0], result.index] : null; | ||
| } | ||
|
|
||
| assert.compareArray(doExec(/𠮷/), ["𠮷", 0], "Basic exec without v flag"); |
There was a problem hiding this comment.
I think it's fine both to keep it or remove it because the behavior of an empty flag is already caught by other tests. I've added it back in the newest revision.
| assert.compareArray(/\P{ASCII}/u.exec(complexText), ["\u{20BB7}"], "Non-ASCII with u flag"); | ||
| assert.compareArray(/\P{ASCII}/v.exec(complexText), ["\u{20BB7}"], "Non-ASCII with v flag"); | ||
|
|
||
| reportCompare(0, 0); |
There was a problem hiding this comment.
| reportCompare(0, 0); |
This undefined function is called in the end of every file. Is this accidental or am I overlooking something?
There was a problem hiding this comment.
This is a mistake, I will remove them.
Thanks for pointing out.
reportCompare is used in the spidermonkey codebase.
|
|
||
| assert.sameValue(doMatch(/x/u), null, "Non-matching regex with u flag"); | ||
| assert.sameValue(doMatch(/x/v), null, "Non-matching regex with v flag"); | ||
| reportCompare(0, 0); |
There was a problem hiding this comment.
| reportCompare(0, 0); |
Same.
| undefined, | ||
| "Non-ASCII with v flag" | ||
| ); | ||
| reportCompare(0, 0); |
There was a problem hiding this comment.
| reportCompare(0, 0); |
Same.
| assert.sameValue(doReplace(/\p{Script=Han}/gu, 'X'), "XaXbX", "Unicode property escapes with u flag"); | ||
| assert.sameValue(doReplace(/\p{Script=Han}/gv, 'X'), "XaXbX", "Unicode property escapes with v flag"); | ||
|
|
||
| reportCompare(0, 0); |
There was a problem hiding this comment.
| reportCompare(0, 0); |
Same.
| // Non-existent pattern | ||
| assert.sameValue(doSearch(/x/u), -1, "Search for non-existent pattern with u flag"); | ||
| assert.sameValue(doSearch(/x/v), -1, "Search for non-existent pattern with v flag"); | ||
| reportCompare(0, 0); |
There was a problem hiding this comment.
| reportCompare(0, 0); |
Same.
test/built-ins/String/prototype/matchAll/regexp-prototype-matchAll-v-u-flag.js
Show resolved
Hide resolved
| assert.sameValue( | ||
| doMatchAll(/(?:)/gu).length, | ||
| 6, | ||
| "Empty matches with u flag" | ||
| ); | ||
|
|
||
| assert.sameValue( | ||
| doMatchAll(/(?:)/gv).length, | ||
| 6, | ||
| "Empty matches with v flag" | ||
| ); |
There was a problem hiding this comment.
These two tests seem to fail on my side on V8, JSC, SpiderMonkey, and Node. This is unexpected, since the second test was there already added in the previous PR #4213.
There was a problem hiding this comment.
Interesting, I think it's because I flat the array of [m[0], m.index] in doMatchAll.
It should be 12 now, I will fix this later.
Thanks for the feedback.
|
Thanks for all the feedback. I greatly appreciate your time and assistance. |
A follow up pr for #4213.