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

Make the $262 object available within the test262 tests #1229

Conversation

p-bakker
Copy link
Collaborator

@p-bakker p-bakker commented May 23, 2022

Closes #1077

PR not yet complete: needs at least the test262.properties file regenerated

@p-bakker
Copy link
Collaborator Author

Added this PR quickly as it might come in handly for #1227

@p-bakker p-bakker marked this pull request as ready for review May 23, 2022 18:57
@p-bakker p-bakker requested a review from rbri May 23, 2022 18:58
Copy link
Collaborator

@rbri rbri left a comment

Choose a reason for hiding this comment

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

LGTM

this.scope = scope;

scope.put("$262", scope, this);
scope.setAttributes("$262", ScriptableObject.DONTENUM);
Copy link
Collaborator

@rbri rbri May 23, 2022

Choose a reason for hiding this comment

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

I think the scope setup should be done in the setup and mot in the ctor.

Copy link
Collaborator Author

@p-bakker p-bakker May 24, 2022

Choose a reason for hiding this comment

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

With setup you mean moving the code into the .buildScope() method?

That would entail duplicating the code between there and the $262.createRealm method, which i don't really like either

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, is see, my problem was as i saw this line

new $262(scope);

i was really confused because i did not expect that this ctor call does some kind of registration in the scope. So from the first look this line looks really useless.

Maybe a factory method that has a hint in the name about the registration?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about now?

@rbri
Copy link
Collaborator

rbri commented May 24, 2022

Any idea why there is no real effect?

@rbri
Copy link
Collaborator

rbri commented May 24, 2022

Btw. i think this is again great step forward for Rhino - THANKS.

@p-bakker
Copy link
Collaborator Author

There is an effect now no? Some previously failing tests are now passing.

I guess you mean why aren't more of the tests that depended on cross-realm passing now? Can't recall, but when I looked into it a while ago, they were failing due to other reasons. I filled for example #1079, because that is an EcmaScript incompatibility when using Symbols cross-realm

@p-bakker
Copy link
Collaborator Author

This PR also needs a squash merge....

@p-bakker p-bakker requested a review from rbri May 24, 2022 08:43
Copy link
Collaborator

@rbri rbri left a comment

Choose a reason for hiding this comment

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

LGTM

@p-bakker
Copy link
Collaborator Author

@rbri great, tnx for the review. Can you squash-merge this PR?

@rbri rbri merged commit b3aac79 into mozilla:master May 24, 2022
@rbri
Copy link
Collaborator

rbri commented May 24, 2022

Yes I can :-)

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.

Implement missing infrastructure required by Test262 test suite
2 participants