Skip to content
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

Fix Regex Character Class Escape Tests #4364

Closed
wants to merge 0 commits into from

Conversation

Aurele-Barriere
Copy link
Contributor

This PR is the follow-up of PR 4195.

Auto-generated tests for character class escapes are not testing what they should.
This commit modifies their generator so that we check, for each character class escape that it matches exactly the characters it's supposed to match, not more and not less.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future reference — when I cleaned up the original test generator I removed this file that modified the regenerate library's object prototype, and instead I turned it into regular functions in index.mjs. I would like to try to minimize the diff in this PR so that it's easier to see what changed, so I've taken the liberty of editing these functions so they don't modify the regenerate prototype and moving them back to index.mjs.

@ptomato
Copy link
Contributor

ptomato commented Mar 13, 2025

@Aurele-Barriere Sorry, I did something that closed this PR by accident when I updated it. I can't reopen it for some reason. Would you mind reopening it?

In any case, I apologize for leaving it for so long.

@Aurele-Barriere
Copy link
Contributor Author

Hi, no worries, here it is again: #4423

@ljharb
Copy link
Member

ljharb commented Mar 13, 2025

@ptomato actually you pushed this PR to main, effectively merging it.

@ljharb
Copy link
Member

ljharb commented Mar 13, 2025

I've just force-pushed those changes out (48bb262 was the head).

@Aurele-Barriere
Copy link
Contributor Author

@ptomato actually you pushed this PR to main, effectively merging it.

If I understand correctly, I don't think this PR has been merged to main, my changes are not in the current files of the main branch of test262 (the regex character class escapes tests are still inadequate).
However, recent test262 commits have been force-pushed to my fork (https://github.com/Aurele-Barriere/test262/tree/main), so I think my changes were deleted by the force-push.
I think it is fine to resume the review from the new pull request, #4423 , created from a new fork

@ljharb
Copy link
Member

ljharb commented Mar 13, 2025

@Aurele-Barriere that's because i just force-pushed them out :-) there's no choice but to resume from the new pull request, so that's fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants