Skip to content

test(es/minifier): move issue_11835 fixture out of terser folder#11840

Merged
kdy1 merged 1 commit intomainfrom
claude/pr-11837-20260502-1657
May 4, 2026
Merged

test(es/minifier): move issue_11835 fixture out of terser folder#11840
kdy1 merged 1 commit intomainfrom
claude/pr-11837-20260502-1657

Conversation

@kdy1
Copy link
Copy Markdown
Member

@kdy1 kdy1 commented May 3, 2026

Per review feedback on #11837, relocate the issue_11835 regression case from tests/terser/compress/sequences/issue_11835 to tests/fixture/issues/11835, where SWC's own (non-Terser-imported) issue fixtures live. Drop the terser-specific output.mangleOnly.js and expected.stdout artifacts.

Follow-up to #11837.

Generated with Claude Code

Per review feedback, relocate the issue_11835 regression case from
tests/terser/compress/sequences/issue_11835 to tests/fixture/issues/11835,
where SWC's own (non-Terser-imported) issue fixtures live. Drop the
terser-specific output.mangleOnly.js and expected.stdout artifacts.

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 3, 2026

⚠️ No Changeset found

Latest commit: 56e389a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@kdy1 kdy1 marked this pull request as ready for review May 3, 2026 08:42
Copilot AI review requested due to automatic review settings May 3, 2026 08:42
@kdy1 kdy1 requested a review from a team as a code owner May 3, 2026 08:42
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 3, 2026

Review

This is a straightforward, well-scoped follow-up to #11837. The relocation matches existing conventions and the deletions are correct.

Verification

  • The new location tests/fixture/issues/11835/ is picked up by custom_fixture in compress.rs:307 (tests/fixture/**/input.js), which only reads config.json, input.js, and output.js. The retained files match exactly what the harness consumes.
  • find_config (compress.rs:291) walks upward, so the per-fixture config.json ({"collapse_vars": true, "sequences": true}) correctly overrides the parent tests/fixture/issues/config.json.
  • The deleted artifacts are obsolete in the new location:
    • expected.stdout was only consumed by terser_exec.rs (tests/terser/compress/**/input.js) and the terser branch of compress.rs:476 (tests/terser/compress/**/input.js).
    • output.mangleOnly.js was only consumed by mangle.rs:67 (tests/terser/**/input.js).
      Neither glob matches the new path.
  • output.js still encodes the fix(es/minifier): preserve for-init sequence order #11837 fix — for(lst = other(), pos = 1; …) keeps lst = other() ahead of pos = 1 in the for-init.

Minor observations (not blocking)

  • Moving out of the terser tree drops the runtime check that the minified program prints 3. That's consistent with how the rest of tests/fixture/issues/ works (snapshot-only), but it does mean a future regression that produces a syntactically different but still wrong output could pass the snapshot if UPDATE=1 were applied carelessly. Not unique to this PR — just worth noting.
  • The config file uses 4-space indentation while many neighbors in tests/fixture/issues/ use 2 spaces. Cosmetic only; both are accepted by the harness.

LGTM — purely test-layout cleanup with no behavior change. ✅

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 56e389a338

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@@ -1 +0,0 @@
3
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve runtime-output assertion for issue_11835

Deleting expected.stdout while relocating this case out of tests/terser/compress/** drops its execution-based semantic check: custom_fixture only compares output.js text/AST and never runs the optimized code, whereas terser_exec explicitly executes and asserts stdout (tests/terser_exec.rs, fixture at lines 41-43 and assertion flow at 93-147). As a result, this regression test no longer verifies the runtime behavior that the original fixture guarded (printing 3), so semantic regressions for this pattern can slip through.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Relocates the issue_11835 minifier regression fixture from the Terser-imported test suite into SWC’s native tests/fixture/issues area, aligning it with the project’s organization for SWC-owned issue regressions.

Changes:

  • Removed the old tests/terser/.../issue_11835 expected artifacts (including Terser-specific output.mangleOnly.js and expected.stdout).
  • Added a new SWC-native fixture under tests/fixture/issues/11835 with input.js, output.js, and a targeted config.json.

Reviewed changes

Copilot reviewed 2 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/swc_ecma_minifier/tests/terser/compress/sequences/issue_11835/output.mangleOnly.js Removes the Terser-specific mangle-only output artifact for this case.
crates/swc_ecma_minifier/tests/terser/compress/sequences/issue_11835/expected.stdout Removes runtime stdout expectation from the Terser fixture location.
crates/swc_ecma_minifier/tests/fixture/issues/11835/input.js Adds SWC-native regression input reproducing the sequence-order side-effect scenario.
crates/swc_ecma_minifier/tests/fixture/issues/11835/output.js Adds the expected SWC minifier output ensuring other() remains ordered before pos = 1 in the for init sequence.
crates/swc_ecma_minifier/tests/fixture/issues/11835/config.json Adds a focused config enabling the relevant optimizations (collapse_vars, sequences).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 3, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 27M (27759688 bytes)

Commit: eaa851d

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 3, 2026

Merging this PR will not alter performance

✅ 219 untouched benchmarks
⏩ 31 skipped benchmarks1


Comparing claude/pr-11837-20260502-1657 (56e389a) with main (9101c71)

Open in CodSpeed

Footnotes

  1. 31 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@kdy1 kdy1 merged commit 3dd3431 into main May 4, 2026
75 of 76 checks passed
@kdy1 kdy1 deleted the claude/pr-11837-20260502-1657 branch May 4, 2026 01:59
@github-actions github-actions Bot added this to the Planned milestone May 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants