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

Add tests for import attributes #3843

Closed
wants to merge 6 commits into from

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 7, 2023

https://github.com/tc39/proposal-import-attributes
The proposal supersedes import assertions, and is at stage 3 conditional on ecma-262 editors reviewing its spec.

Closes #3829

TODO (Maybe in a separate PR, to keep this one only about renaming and copying tests):

  • Test using both variants at the same time, both in dynamic import and in static import

@nicolo-ribaudo nicolo-ribaudo requested a review from a team as a code owner June 7, 2023 14:29
flags: [module]
---*/

import x from './import-assertion-1_FIXTURE.js' with {test262:'',};
Copy link
Member Author

Choose a reason for hiding this comment

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

With the new proposal, unknown import attributes should throw rather than being ignored. Should we require runner to define a no-op test262 attribute that we can use for testing purposes?

Copy link

Choose a reason for hiding this comment

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

Not sure how the runner would add a new attribute -- the set of expected attributes isn't user-extensible is my understanding.

For this test case, it seems like type: 'js' would be sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how the runner would add a new attribute -- the set of expected attributes isn't user-extensible is my understanding.

It's defined at the embedder level.

For this test case, it seems like type: 'js' would be sufficient?

In this specific case it would, however:

  • the type attribute is not defined in ECMA-262. It's defined by the JSON modules proposal, but only with type: "json"
  • we need multiple attributes for some tests, for example for testing the order properties in dynamic import are accessed.

Copy link

Choose a reason for hiding this comment

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

So hosts will have to implement 'test262-only' attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like it'd be prohibitive for runners that aren't a JS engine's shell...

@mgaudet
Copy link

mgaudet commented Jul 11, 2023

Is there a plan for testing the deprecated assert syntax? Perhaps a feature-tag and a small set of smoke tests for that?

Nevermind -- I see this is still keeping the old tag :)

Comment on lines +32 to +43
// Define a property on the global "this" value so that the effect of the
// expected IdentifierReference can be observed.
Object.defineProperty(globalThis, 'assert', {
get: function() {
callCount += 1;
}
});

import x from './import-assertion-1_FIXTURE.js'
with
{test262:''};

Copy link

Choose a reason for hiding this comment

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

This test currently is sort of splinched; should define with and ensure callCount === 0 right?


var thrown = new Test262Error();
var options = {
get assert() {
Copy link

Choose a reason for hiding this comment

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

s/assert/with/




test262



:with
Copy link

Choose a reason for hiding this comment

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

This with doesn't seem to belong here.



''



};
import './import-assertion-2_FIXTURE.js' assert
Copy link

Choose a reason for hiding this comment

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

s/assert//



''



};
export * from './import-assertion-3_FIXTURE.js' assert
Copy link

Choose a reason for hiding this comment

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

s/assert/with/ right?

@ptomato
Copy link
Contributor

ptomato commented Oct 31, 2023

I don't think this was supposed to be auto-closed by #3921!

@ptomato ptomato reopened this Oct 31, 2023
@nicolo-ribaudo
Copy link
Member Author

Yes it was :) This PR was split in separate PRs to make it easier to review.

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.

Tests for import attributes
4 participants