test(parser): combine test cases for let as identifier#16861
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merge activity
|
There was a problem hiding this comment.
The consolidation looks correct and the snapshot updates are consistent with the file moves, but the new fixture names/comments are now slightly misleading because they bundle optional chaining cases under “member expression”. Consider renaming or splitting to keep fixtures narrowly scoped and easier to maintain/search.
Additional notes (3)
- Maintainability |
tasks/coverage/misc/fail/let-member-expression.js:1-12
The file comment says it tests member/call expressions, but it also includes several optional chaining cases. That’s fine functionally, but it makes the fixture name/comment slightly misleading and harder to grep later when someone is looking specifically for optional chaining behavior.
Consider either splitting optional chaining back out or renaming/commenting the fixture to reflect both concerns (e.g., member/call and optional chaining) so future changes don’t accidentally drop coverage.
-
Maintainability |
tasks/coverage/misc/fail/let-member-expression.js:1-3
'use strict';is redundant in a file that is explicitly described as a module fixture, since ES modules are always strict mode. If the harness treats.jsinmisc/failas a module viasourceType: "module", this line is noise and can slightly obscure what is being tested (reserved-word parsing vs. strictness). If, however, the harness relies on'use strict'to force strictness for.jsinputs, the comment should say that explicitly to avoid future confusion. -
Maintainability |
tasks/coverage/misc/pass/let-member-expression.cjs:1-10
In the passing.cjsfixture, the comment says “in script”, but the file also contains optional chaining. Depending on what this coverage suite is intended to assert, optional chaining may require a minimum language version/feature flag in some parsers.
If the test runner’s configuration ever varies (e.g., running with older ECMAScript targets), this fixture could become an accidental coupling between “script vs module let identifier rules” and “optional chaining enabled”.
Summary of changes
What changed
This PR consolidates and renames parser coverage fixtures around using the reserved keyword let as an identifier in member/call expressions.
- New negative fixture:
tasks/coverage/misc/fail/let-member-expression.js- Combines prior failing cases from
let-optional-chaining.jsandoxc.jsinto one module/strict-mode fixture.
- Combines prior failing cases from
- New positive fixture:
tasks/coverage/misc/pass/let-member-expression.cjs- Consolidates prior passing optional chaining script fixture into a single
.cjsscript fixture.
- Consolidates prior passing optional chaining script fixture into a single
- Removed redundant/unclear fixtures:
- Deleted
tasks/coverage/misc/fail/let-optional-chaining.js - Deleted
tasks/coverage/misc/fail/oxc.js(previously non-descriptive) - Deleted
tasks/coverage/misc/pass/let-optional-chaining.cjs
- Deleted
- Snapshot update:
tasks/coverage/snapshots/parser_misc.snap- Updates counts (
Negative Passed: 122/122→121/121) and rewires expected error locations to the new consolidated file/line numbers.
- Updates counts (
e4d389c to
75f9800
Compare

Follow-on after #16840 and #16857. We had a few test cases relating to the same thing, and one of them called the undescriptive
oxc.js. Combine them, and rename the test case files with a more accurate name.