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

Remove SM staging tests wrongly testing invalid LHS #4345

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

Conversation

nicolo-ribaudo
Copy link
Member

It is an early error if the LHS of:

  • ForOfStatement
  • UpdateExpression
  • AssignmentExpression
  • DestructuringAssignmentTarget is not simple (i.e. not a function call).

It is an early error if the LHS of:
- ForOfStatement
- UpdateExpression
- AssignmentExpression
- DestructuringAssignmentTarget
is not simple (i.e. not a function call).
@ljharb
Copy link
Member

ljharb commented Dec 13, 2024

cc @dminor since at least one of these is marked as a regression test

@Ms2ger Ms2ger self-assigned this Dec 13, 2024
@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 13, 2024

I'll deal with these next week

@Ms2ger
Copy link
Contributor

Ms2ger commented Dec 17, 2024

The spec is wrong here: tc39/ecma262#2193

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Dec 17, 2024

These tests can be added once the spec is updated. We cannot have tests for something that has never even been presented in plenary, and even if we did then we'd need to have them with the appropriate feature flag and not mixing the feature with other unrelated assertions (in the for-of-iterator-close.js case).

@ljharb
Copy link
Member

ljharb commented Dec 17, 2024

I’m not sure that level of rigor needs to hold for the staging directory.

@nicolo-ribaudo
Copy link
Member Author

nicolo-ribaudo commented Dec 18, 2024

@ljharb I agree that there doesn't need to be much rigor to merge tests in the staging directory. However, it should be possible to fix them when needed.

The goal of the staging directory is for implementations to be able to share tests with each other before that they are "good enough" to be merged in the main test262 corpus. In this case we have already two consumers of test262 that correctly implement the spec and stumbled upon problems with the tests touched by this PR, because they are not what the spec asks us to implement.

Once somebody reports that a test is wrong, the test should be removed or fixed. This is the staging process working! The wrong test doesn't need to be caught by review of test262 maintainers, but it can be reviewed asynchronously by other implementations after that it is merged.

In this specific case, there is an open PR in the ecma262 repo that would make some of the wrong tests in this PR (but not the for-of one) correct. That PR has not even been presented to the committee yet, and looking at the comments in that PR there is no agreement on what the change should be: it's simply not possible to write tests for it yet.

However, given that the tests exist and maybe one day the spec will actually match what these tests say, we could add a feature flag like nonstandard-lhs-call, and consumers of test262 would know that they can skip tests marked as nonstandard-lhs-call. Again, I'm not saying that this should have done before merging the tests to staging, but that tests are meant to be refined once they are merged to staging.


EDIT: This was found by Babel and Porffor while running the updated tests, but it seems like they are not the only ones not passing it:

image image image

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