Skip to content

test(es/minifier): Add execution tests for issue #11517#11530

Merged
kdy1 merged 2 commits intomainfrom
issue-11517-tests
Feb 6, 2026
Merged

test(es/minifier): Add execution tests for issue #11517#11530
kdy1 merged 2 commits intomainfrom
issue-11517-tests

Conversation

@kdy1
Copy link
Member

@kdy1 kdy1 commented Feb 4, 2026

Summary

This PR adds comprehensive test coverage for PR #11526.

Test Coverage

Core Issue Tests:

  • Original reproduction case from issue
  • Simplified version
  • Verification that correct merging still works

Variable Type Variations:

  • const, let, var declarations
  • Multiple local variables with same names

Statement Type Variations:

  • return, continue, break, throw statements
  • Try-catch blocks, labeled statements

Data Structure Variations:

  • Objects, Arrays, Maps, Sets, WeakMap, WeakSet
  • TypedArrays, DataView, Proxies, Reflect API

Function Variations:

  • Regular functions, arrow functions
  • Async functions, generators
  • Class methods, constructors, closures, IIFEs

Control Flow Variations:

  • Nested if statements, if-else-if chains
  • For/while/do-while/for-of/for-in loops
  • Switch statements

Real-World Patterns:

  • HTTP status codes, form validation
  • Redux-like reducers, i18n translations
  • Theme systems, logging, API responses
  • File handlers, command parsers, permissions

Test plan

  • All 113 new tests pass
  • All existing execution tests still pass

🤖 Generated with Claude Code

Add 113 execution tests to verify the fix for incorrect if statement
merging when local variables have different values.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@kdy1 kdy1 requested a review from a team as a code owner February 4, 2026 11:29
Copilot AI review requested due to automatic review settings February 4, 2026 11:29
@changeset-bot
Copy link

changeset-bot bot commented Feb 4, 2026

⚠️ No Changeset found

Latest commit: 6b543dc

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

@kdy1 kdy1 enabled auto-merge (squash) February 4, 2026 11:30
@kdy1 kdy1 added this to the Planned milestone Feb 4, 2026
@claude

This comment has been minimized.

Copy link
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

This PR extends the minifier’s execution test suite to comprehensively cover the fix for issue #11517 (incorrect merging of if statements when their local variables differ), ensuring both the original regression and a wide matrix of variants are protected. The changes are entirely in test code and exercise many language and control-flow patterns to validate that if-merging only occurs when it is semantics-preserving.

Changes:

  • Add an execution test for the original reproduction of #11517 and a simplified core variant.
  • Add a large family of execution tests spanning variable kinds, control-flow constructs, data structures, and modern JS features, all structured to catch incorrect if-merging when local variable values differ.
  • Include a positive test that verifies that merging still works when branch-local values are intentionally identical.

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

kodiakhq[bot]
kodiakhq bot previously approved these changes Feb 4, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2026

Binary Sizes

File Size
swc.linux-x64-gnu.node 28M (28689992 bytes)

Commit: 8849810

@codspeed-hq
Copy link

codspeed-hq bot commented Feb 4, 2026

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing issue-11517-tests (6b543dc) with main (ff6cf88)

Summary

✅ 184 untouched benchmarks

@kdy1 kdy1 disabled auto-merge February 4, 2026 13:26
@kdy1
Copy link
Member Author

kdy1 commented Feb 5, 2026

@claude Remove unnecessary tests. e.g. Duplicate (verified by other test case)

@claude

This comment has been minimized.

Remove redundant tests that were functionally equivalent variations of
the same core bug test. Kept essential tests covering:
- Original and simplified reproduction cases
- Verification that valid merging still works
- Variable types (const, let, var)
- Control flow (continue, break, throw)
- Class methods and closures
- Real-world HTTP status pattern

Co-authored-by: Donny/강동윤 <kdy1@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ kdy1
❌ github-actions[bot]
You have signed the CLA already but the status is still pending? Let us recheck it.

@claude
Copy link
Contributor

claude bot commented Feb 5, 2026

PR Review: test(es/minifier): Add execution tests for issue #11517

Overview

This PR adds execution tests to verify the fix for issue #11517, which involves incorrect if-statement merging when local variables have different values. The tests are well-structured and cover a good variety of scenarios.

👍 Strengths

  1. Good test coverage: The tests cover multiple important variations:

    • Original reproduction case from the issue
    • Different variable declaration types (const, let, var)
    • Different control flow statements (return, continue, break, throw)
    • Class methods and closures
    • Real-world patterns (HTTP status codes)
  2. Clear documentation: Each test has a descriptive doc comment explaining what it tests, which is actually an improvement over some existing tests in the file.

  3. Consistent style: Uses run_default_exec_test() appropriately for most tests, which is the standard pattern for execution tests with default minifier config.

  4. Edge case consideration: Including issue_11517_same_values_should_merge is a great decision - it ensures the fix doesn't break valid merging behavior.

⚠️ Issues to Address

  1. PR description inaccuracy: The PR description claims "113 execution tests" but only 11 tests are actually added. This should be corrected to avoid confusion.

  2. PR fix(es/minifier): Inline before merge if #11526 is still open: This test PR references PR fix(es/minifier): Inline before merge if #11526 as the fix, but fix(es/minifier): Inline before merge if #11526 hasn't been merged yet. Consider:

💡 Suggestions (Optional)

  1. Minor typo in doc comment: In issue_11517_original, the comment says "Original reproduction case" but the test differs slightly from the original issue (uses 'A error occurred' vs 'A error occured' in the issue). This is fine since it tests the same pattern, but consistency could help.

  2. Consider edge cases: A few additional edge cases that could strengthen coverage (optional for future):

    • Nested if statements with different local variable values at different nesting levels
    • Switch statements with similar patterns

✅ Verdict

The code quality is good and follows repository conventions. The tests are valuable regression tests for issue #11517.

Recommendation: Fix the PR description (113 → 11 tests) and coordinate with PR #11526 for merging order.


Reviewed by Claude Code

@kdy1 kdy1 merged commit 01b3b64 into main Feb 6, 2026
187 of 188 checks passed
@kdy1 kdy1 deleted the issue-11517-tests branch February 6, 2026 04:12
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.

2 participants