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 a test for decorator context names #4022

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

@mansona mansona requested a review from a team as a code owner March 22, 2024 18:03
Comment on lines 24 to 25
// TODO these two throw an error with SyntaxError: Unexpected token ILLEGAL but they pass on babel's test suite. should we remove them?
@dec static accessor 2n;
@dec static accessor [3n];
Copy link
Author

Choose a reason for hiding this comment

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

These are currently failing when I use the babel-test262-runner to test them but are working upstream in babel so I don't know if I should keep these two cases 🤔 thoughts?

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Thanks! I looked at these and they seem correct according to the current text of the decorators proposal. I don't really have a good sense of whether we already have coverage for this stuff so it'd be helpful if we could get a look from someone who's been involved with decorator tests before.

About the bigint property names, the question is, are they correct according to the spec. As far as I can tell, they are.

Here's the relevant grammar, with [Yield, Await] elided:

  • ClassElement : DecoratorListopt static FieldDefinition ;
  • FieldDefinition : accessor [no LineTerminator here] ClassElementName Initializeropt
  • ClassElementName : PropertyName
  • PropertyName : LiteralPropertyName
  • LiteralPropertyName : NumericLiteral
  • NumericLiteral : DecimalBigIntegerLiteral

(similar for PropertyName : ComputedPropertyName but with a longer path through all of the expression precedence elements)

So I don't think the SyntaxError is correct there.

test/language/statements/class/decorator/context-name.js Outdated Show resolved Hide resolved
@mansona
Copy link
Author

mansona commented Mar 23, 2024

Thanks for the review @ptomato 🎉

I've updated the message and removed my TODO item for removing the seemingly valid syntax 👍 is there something that I need to do to the frontmatter to mark it that it's currently expected to fail?

I don't really have a good sense of whether we already have coverage for this stuff

So I have 2 thoughts on this. If adding this test has identified a legitimate error that should have passed then this is clearly adding coverage for something that wasn't already covered 😂 But looking at the files that are already in the suite for decorators it seems that there are only syntax tests that have the comment This file was procedurally generated from the following sources so I'm assuming there are no hand-crafted tests that have any assertions, or at least that's how it seems from my investigations.

But anyway, I'm adding these tests with the help of @pzuraq so I'm sure she'll be the best person to comment on this 😂

@nicolo-ribaudo
Copy link
Member

Decorators coverage right now is something like 0.1%, so thank you very much!

If you plan to submit more tests, it would be incredibly useful if you could write a "testing plan" (like #3725) so that it's easier to tell what's covered and what's not.

@Ms2ger
Copy link
Contributor

Ms2ger commented Mar 25, 2024

Please add a copyright header as well

@ptomato
Copy link
Contributor

ptomato commented Mar 25, 2024

is there something that I need to do to the frontmatter to mark it that it's currently expected to fail?

@mansona There are no expected-to-fail tests, so nothing to do on your end. (The way it works is, a hypothetical JS implementation that correctly implements the spec plus all stage-3 proposals must pass all test262 tests. Implementations maintain their own expected-to-fail lists separately.)

@mansona
Copy link
Author

mansona commented Mar 30, 2024

Please add a copyright header as well

So I was wondering about that one 🤔 As I said in the description I copied the test from the babel test suite, so should I set the copyright header to the original author? Is there any guidance on that?

@ptomato
Copy link
Contributor

ptomato commented Apr 1, 2024

As I said in the description I copied the test from the babel test suite, so should I set the copyright header to the original author? Is there any guidance on that?

Copyrights are annoying to deal with, sorry. Ideally the Babel test file would have its own copyright header, but it doesn't. You'd have to ask the Babel project if the copyright line in babel/LICENSE ("Copyright (c) 2014-present Sebastian McKenzie and other contributors") applies to everything in the babel repo. If not, then you'd have to talk to the authors of that file figure out who owns the copyright.

(Public service announcement: Open source projects, please require license and copyright headers in every file in the repo! There are tools such as REUSE that you can drop into your CI and make this easy, or you can roll your own like test262 does 😄)

@nicolo-ribaudo
Copy link
Member

Hey, could you use "The Babel Team"? Unfortunately different packages in our repo have different author/copyright owner, so the best thing to do is to look for the author field in the relevant package.json (https://github.com/babel/babel/blob/dd699a8509f02efd7e5a45d49cf24b757a59e4a2/packages/babel-plugin-proposal-decorators/package.json#L4C14-L4C30)

@pzuraq
Copy link

pzuraq commented Apr 3, 2024

@nicolo-ribaudo I've made a test plan in this issue: #4042

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.

5 participants