test262 icon indicating copy to clipboard operation
test262 copied to clipboard

Add tests for the RegExp Modifiers proposal

Open rbuckton opened this issue 1 year ago • 5 comments

This adds tests for the Stage 3 RegExp Modifiers proposal.

rbuckton avatar Nov 15 '23 19:11 rbuckton

Is there any action I need to take to help move this along? I'd like to unblock V8 if possible.

rbuckton avatar Jan 17 '24 19:01 rbuckton

Thanks for asking! What I think will need to happen here, that you could help with, is:

  • Compare this PR with #3807 (which contains syntax tests for this feature) to see if there are any duplicated parts, and if so, which version to use
  • Check if this PR covers the remainder of the cases from the testing plan in #3756

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?

ptomato avatar Jan 18 '24 20:01 ptomato

It looks like the following files in this PR are already covered by #3807:

  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-modifiers-duplicate-add-modifiers.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-modifiers-invalid-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-modifiers-invalid-flag-in-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-both-empty.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-duplicate-add-modifiers.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-duplicate-remove-modifiers.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-invalid-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-invalid-flag-in-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-invalid-flag-in-remove-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-invalid-remove-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/add-remove-modifiers-same-modifier-in-both.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-modifiers-duplicate-add-modifiers.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-modifiers-invalid-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-modifiers-invalid-flag-in-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-both-empty.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-duplicate-add-modifiers.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-duplicate-remove-modifiers.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-invalid-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-invalid-flag-in-add-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-invalid-flag-in-remove-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-invalid-remove-modifier.js
  • test/built-ins/RegExp/regexp-modifiers/syntax/invalid/regexp-constructor-add-remove-modifiers-same-modifier-in-both.js

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:

  • Valid syntax
    • [x] Basic regular expression flags parse correctly
      • [ ] Source text uses unicode escape sequences to express code points i, m, s
    • [x] Source text with any valid combination of flags or arithmetic flags - reasonable to enumerate
  • Behavior
    • [x] Disabling flag in subexpression behaves correctly when corresponding top-level flag is and isn't already set
    • [x] Enabling flag in subexpression behaves correctly when corresponding top-level flag is and isn't already set
    • [x] Constructing a RegExp from a literal but changing flags by an argument to the RegExp constructor does (or does not) correctly change behavior of a subexpression that enables or removes flags.
    • i
      • [x] Ignore case applies appropriately inside subexpression, but not outside; when turned on, off, and when nested inside a subexpression that has previously modified behavior
      • [x] Behavior as normal when other flags modified but i flag not modified
      • Callers of Canonicalize:
        • [x] Backreferences ignore case in captures
        • [x] Individual characters ignore case
        • [x] Character sets ignore case
        • [x] Character escapes ignore case
        • [x] Character class escapes ignore case
        • [x] \w class, \b, \B all ignore case
    • m
      • [x] ^ and $ apply appropriately inside subexpression, but not outside; when turned on, off, and when nested inside a subexpression that has previously modified behavior
    • s
      • [x] . applies appropriately inside subexpression, but not outside; when turned on, off, and when nested inside a subexpression that has previously modified behavior
    • [x] Subexpressions with flags set do not cause RegExp()....flags or /.../.flags to have the flags set, e.g. (new RegExp("(?i:a)")).flags does not include i.
    • [x] ^ for RegExp.prototype.dotAll, .multiline, ignoreCase

I'll look into adding the missing test cases over the next few days.

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?

I reviewed #3807, but it looks like it's blocked by a test generation issue.

rbuckton avatar Feb 13 '24 22:02 rbuckton

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:

Source text uses unicode escape sequences to express code points i, m, s

This is listed as a test for valid syntax, but as far as I am aware /(?\u0069:)/ should be a syntax error and should be added to #3807.

rbuckton avatar Feb 14 '24 21:02 rbuckton

@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 avatar Feb 15 '24 01:02 ioannad

@ioannad I think the only thing missing between this and #3807 is this:

Source text uses unicode escape sequences to express code points i, m, s

Though I would argue the test plan should indicate that this is not supported, i.e.

Source text cannot use unicode escape sequences to express code points i, m, s

Since #3807 seems mostly to cover syntax errors, it seems that would be a better place for this test.

rbuckton avatar Feb 27 '24 15:02 rbuckton

@gibson042: As a reviewer for the proposal, I'd appreciate a review of the tests if you have the time.

rbuckton avatar Feb 27 '24 15:02 rbuckton