Skip to content

fix(v3): mapsEqual test helper panics on slice values#5402

Merged
leaanthony merged 1 commit into
masterfrom
fix/maps-equal-slice-panic
May 11, 2026
Merged

fix(v3): mapsEqual test helper panics on slice values#5402
leaanthony merged 1 commit into
masterfrom
fix/maps-equal-slice-panic

Conversation

@leaanthony
Copy link
Copy Markdown
Member

@leaanthony leaanthony commented May 11, 2026

Summary

  • mapsEqual in v3/internal/commands/build-assets_test.go compares leaf values with ==, which panics at runtime when the value is a slice ([]any).
  • TestPreserveOriginallyEmptyContainers/originally_empty_array_is_preserved deliberately puts an empty []any into a map[string]any, hitting that line and panicking — which takes down the entire internal/commands package test run on every OS, blocking unrelated PRs (e.g. fix(v3/examples): update go.sum files for Windows webview2 dependency #5400).
  • Replace just that leaf comparison with reflect.DeepEqual. Nested-map recursion and the len(a) != len(b) early-out are preserved, so nil-vs-empty semantics are unchanged.

Diff

-		} else if av != bv {
+		} else if !reflect.DeepEqual(av, bv) {
 			return false
 		}

Test plan

  • go test -count=1 ./v3/internal/commands/... — passes locally (was panicking before).
  • CI green on ubuntu / windows / macOS Go test jobs.

Summary by CodeRabbit

  • Tests
    • Enhanced equality comparison for complex data structures in build-assets validation
    • Added test coverage to verify proper preservation of originally empty containers during sanitization

Review Change Stack

The mapsEqual helper in build-assets_test.go compared map values with
==, which panics at runtime when a value is a slice. TestPreserveOriginallyEmptyContainers/originally_empty_array_is_preserved
triggers exactly that case, taking down the entire internal/commands
package test run on every platform and blocking unrelated PRs.

Use reflect.DeepEqual for the leaf value comparison so slice values
compare correctly. Nested map handling and the nil/empty-tolerant len
check are unchanged.
Copilot AI review requested due to automatic review settings May 11, 2026 10:19
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6c404e7b-4473-413f-b95d-863d4467ec76

📥 Commits

Reviewing files that changed from the base of the PR and between a3d392e and 5d4086d.

📒 Files selected for processing (1)
  • v3/internal/commands/build-assets_test.go

Walkthrough

This PR enhances the test file build-assets_test.go by adding deep equality comparison support and a new test case. The reflect package is imported, the mapsEqual helper is updated to use reflect.DeepEqual, and a table-driven test is added to validate how sanitizePlistDict handles originally-empty versus post-removal-empty containers.

Changes

Test Improvements for Plist Dictionary Sanitization

Layer / File(s) Summary
Reflect Import
v3/internal/commands/build-assets_test.go
Adds reflect package import to enable deep equality checks in test utilities.
Equality Helper Enhancement
v3/internal/commands/build-assets_test.go
Updates mapsEqual helper to use reflect.DeepEqual for non-map value comparisons instead of direct inequality, improving handling of nested/non-primitive values.
Container Preservation Test
v3/internal/commands/build-assets_test.go
Adds TestPreserveOriginallyEmptyContainers, a table-driven test that verifies sanitizePlistDict preserves containers that were originally empty while dropping those that become empty only after template stub removal.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A rabbit hops through test code neat,
With reflect to make comparisons sweet,
Deep equality now shines so bright,
Empty containers tested right,
Test coverage takes its flight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and specifically describes the main change: fixing a panic in the mapsEqual test helper when it encounters slice values.
Description check ✅ Passed The description provides clear context, the specific problem, the solution, and a test plan, though some template sections (issue link, type of change checkbox, testing checklist) are not fully completed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/maps-equal-slice-panic

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.1)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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

This PR fixes a runtime panic in the mapsEqual test helper used by v3/internal/commands tests when map leaf values are slices (e.g. []any{}), by switching the leaf comparison from == to reflect.DeepEqual.

Changes:

  • Import reflect in build-assets_test.go.
  • Replace leaf value comparison av != bv with !reflect.DeepEqual(av, bv) in mapsEqual, while preserving the existing nested-map recursion and length checks.

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

@leaanthony leaanthony merged commit 446ba94 into master May 11, 2026
63 checks passed
@leaanthony leaanthony deleted the fix/maps-equal-slice-panic branch May 11, 2026 10:46
leaanthony pushed a commit to taliesin-ai/wails that referenced this pull request May 11, 2026
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.

2 participants