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

Update RegExp property tests per Unicode v14 #3199

Merged
merged 1 commit into from
Sep 30, 2021

Conversation

mathiasbynens
Copy link
Member

mathiasbynens added a commit to mathiasbynens/ecma262 that referenced this pull request Sep 14, 2021
The following new values for the already-supported properties `Script` and `Script_Extensions` are added:

- Cypro_Minoan (Cpmn)
- Old_Uyghur (Ougr)
- Tangsa (Tnsa)
- Toto (Toto)
- Vithkuqi (Vith)

Issue: tc39#2514
Tests: tc39/test262#3199
@jugglinmike
Copy link
Contributor

Thanks, @mathiasbynens! I'd like to help, but I frankly don't know how to constructively review tests like this. Do you have any thoughts about how a reviewer would validate your patch?

@anba: if this looks good to you, then I'd be willing to merge even before I'm ready to review personally.

@mathiasbynens
Copy link
Member Author

That's a good question, and to be honest the answer isn't crystal clear to me either.

The upstream change for the test generator is here: mathiasbynens/unicode-property-escapes-tests@a38f682#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519 which basically defers to other Unicode data-related packages. Tracing it all the way back to the source, we end up at node-unicode/node-unicode-data@243f19e#diff-f67dd27999ce20ea2e7a12a007c56ba704e590cb5bc0bef935928830e1df7a7b which affected the https://github.com/node-unicode/unicode-14.0.0 dependency. Perhaps you'd rather review those changes, and trust that the existing generator still does the right thing when given the right inputs based on the Unicode Standard?

As a double-check, we could also wait until a JS engine updates to Unicode v14 and then verify that they pass all the tests.

@anba
Copy link
Contributor

anba commented Sep 27, 2021

@anba: if this looks good to you, then I'd be willing to merge even before I'm ready to review personally.

Yes, the changes look good to me.

As a double-check, we could also wait until a JS engine updates to Unicode v14 and then verify that they pass all the tests.

I've built SpiderMonkey with Unicode 14 support and verified that all tests pass.

Copy link
Contributor

@jugglinmike jugglinmike left a comment

Choose a reason for hiding this comment

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

That all sounds good--my thanks to both of you! I've left a note about this on the original issue for Unicode property escapes (which was still open, curiously).

@jugglinmike jugglinmike merged commit 04ad519 into tc39:main Sep 30, 2021
@mathiasbynens mathiasbynens deleted the unicode-14-property-escapes branch September 30, 2021 16:44
bakkot pushed a commit to mathiasbynens/ecma262 that referenced this pull request Jan 5, 2022
The following new values for the already-supported properties `Script` and `Script_Extensions` are added:

- Cypro_Minoan (Cpmn)
- Old_Uyghur (Ougr)
- Tangsa (Tnsa)
- Toto (Toto)
- Vithkuqi (Vith)

Issue: tc39#2514
Tests: tc39/test262#3199
ljharb pushed a commit to mathiasbynens/ecma262 that referenced this pull request Jan 5, 2022
…c39#2515)

The following new values for the already-supported properties `Script` and `Script_Extensions` are added:

- Cypro_Minoan (Cpmn)
- Old_Uyghur (Ougr)
- Tangsa (Tnsa)
- Toto (Toto)
- Vithkuqi (Vith)

Issue: tc39#2514
Tests: tc39/test262#3199
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants