-
Notifications
You must be signed in to change notification settings - Fork 454
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 tests for the RegExp Modifiers proposal #3960
Conversation
060972b
to
c2ef6af
Compare
Is there any action I need to take to help move this along? I'd like to unblock V8 if possible. |
Thanks for asking! What I think will need to happen here, that you could help with, is:
Also if we could get a look from a proposal champion or someone else familiar with the proposal, that would help. In this case it probably doesn't make sense for you to review your own PR, and it looks like you're the only champion, but maybe one of the proposal's stage 3 reviewers could lend a hand? |
It looks like the following files in this PR are already covered by #3807:
Since #3807 is more comprehensive for those cases, I would defer to that PR and will remove the duplicates from this PR.
This covers some, but not all, of the remaining test cases:
I'll look into adding the missing test cases over the next few days.
I reviewed #3807, but it looks like it's blocked by a test generation issue. |
The last commit should cover the majority of the remaining portions of the test plan, and removes tests which are redundant with #3807. The only thing not yet covered is:
This is listed as a test for valid syntax, but as far as I am aware |
@rbuckton about the overlapping tests between this and #3807 , I think we should prioritise landing the tests in this PR first. So I will spend the next few days taking a look at this PR and try to make sure we can land it asap. If there are parts of the test plan in #3756 that are not covered here, then we can think of a follow up PR. I could even offer to create such a follow up, unless you prefer to do it. |
@ioannad I think the only thing missing between this and #3807 is this:
Though I would argue the test plan should indicate that this is not supported, i.e.
Since #3807 seems mostly to cover syntax errors, it seems that would be a better place for this test. |
@gibson042: As a reviewer for the proposal, I'd appreciate a review of the tests if you have the time. |
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.
Apart from the suggested changes, looks good to me. I am not sure whether the frontmatter should be more customised to each test, but it's probably not that important, and I think it's good to be merged as is, modulo suggestions.
...xp/regexp-modifiers/syntax/valid/add-and-remove-modifiers-can-have-empty-remove-modifiers.js
Outdated
Show resolved
Hide resolved
test/built-ins/RegExp/regexp-modifiers/syntax/valid/remove-modifiers-when-set-as-flags.js
Outdated
Show resolved
Hide resolved
test/built-ins/RegExp/regexp-modifiers/add-ignoreCase-affects-slash-upper-p.js
Outdated
Show resolved
Hide resolved
test/built-ins/RegExp/regexp-modifiers/add-ignoreCase-affects-slash-upper-p.js
Outdated
Show resolved
Hide resolved
Based on PR tc39#3807 which had generated these tests from templates, but was stuck due to issue tc39#3808. Since these tests are a shipping blocker for v8, and tc39#3808 seems not straight-forward to fix, I modified the generated tests and removed the templates. One test is missing, as noted by @rbuckton in tc39#3960 (comment) to be added in a followup commit.
…m, s As suggested in tc39#3960 (comment)
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.
My review is more superficial but Ioanna's detailed review makes me confident we can merge this. Here's the maintainer stamp. @gibson042 if you still plan to do a review, give a shout by tomorrow, otherwise let's plan to merge this.
assert(re2.test("\u017f"), "\\b should match after \u017f"); | ||
assert(re2.test("\u212a"), "\\b should match after \u212a"); |
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.
Wow, this was a spicy one. Good that you included it.
…m, s As suggested in tc39#3960 (comment)
…m, s As suggested in tc39#3960 (comment)
…m, s As suggested in #3960 (comment)
d438696
to
47b1f5e
Compare
This adds tests for the Stage 3 RegExp Modifiers proposal.