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.

Sign up for GitHub

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

Jump to bottom

Fix phase on some negative module tests #3832

Merged
merged 2 commits into from
Jun 10, 2023
Merged

Fix phase on some negative module tests #3832

merged 2 commits into from
Jun 10, 2023

Conversation

jedel1043
Copy link
Contributor

@jedel1043 jedel1043 commented May 22, 2023 •
edited
Loading

A couple of negative tests had an incorrect phase tagged, since their errors happen at linking time, which is part of the "resolution" phase. However, they were marked as happening at runtime.

This PR fixes those tests and additionally adds a $DONOTEVALUATE marker to them.

@jedel1043 jedel1043 requested a review from a team as a code owner May 22, 2023 07:19
@jedel1043 jedel1043 mentioned this pull request May 22, 2023
Merged
@jedel1043 jedel1043 force-pushed the fix-resolution-phase branch from c37a3db to 8d48805 Compare May 29, 2023 10:05
@jedel1043 jedel1043 force-pushed the fix-resolution-phase branch from 8d48805 to 7d2bb4d Compare June 1, 2023 07:06
jugglinmike
jugglinmike approved these changes Jun 10, 2023
View reviewed changes
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.

Thanks for the fix!

Out of curiosity: do you have an environment that was failing the erroneous version of these tests? Or did you spot the mistake through careful reading?

@jugglinmike jugglinmike enabled auto-merge (squash) June 10, 2023 22:09
@jugglinmike jugglinmike disabled auto-merge June 10, 2023 22:09
@jugglinmike jugglinmike enabled auto-merge (squash) June 10, 2023 22:10
@jugglinmike jugglinmike merged commit c4642dd into tc39:main Jun 10, 2023
@jedel1043
Copy link
Contributor Author

jedel1043 commented Jun 10, 2023 •
edited
Loading

Or did you spot the mistake through careful reading?

I wish 😅

Out of curiosity: do you have an environment that was failing the erroneous version of these tests?

I'm part of the team developing Boa, an experimental JS engine written in Rust. We found out about the bugs when we were finishing our implementation of the ES modules spec
(boa-dev/boa#2955)

Internally, we have a testing tool that uses test262 to know our current conformance at any point while developing.

@jedel1043 jedel1043 deleted the fix-resolution-phase branch June 10, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jugglinmike jugglinmike jugglinmike approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jedel1043 @jugglinmike @ptomato