test(v3): add comprehensive unit tests for pkg/application#4827
test(v3): add comprehensive unit tests for pkg/application#4827leaanthony merged 7 commits intov3-alphafrom
Conversation
Add 11 new test files to improve test coverage of the pkg/application package from 13.6% to 17.7%. New test files: - context_test.go: Context struct operations - services_test.go: Service management and lifecycle - parameter_test.go: Parameter and CallError types - dialogs_test.go: Dialog utilities and button methods - webview_window_options_test.go: Window options and constants - application_options_test.go: ChainMiddleware and app config - keys_test.go: Keyboard accelerator parsing - single_instance_test.go: Single instance management and encryption - menuitem_internal_test.go: Menu item internal functions - menu_internal_test.go: Menu internal functions - screenmanager_internal_test.go: Screen geometry and transformations The 40% target was not fully achievable because ~50% of the codebase is platform-specific code that can only be tested on respective platforms. Tests focus on pure Go logic, utility functions, and data structures that can be tested cross-platform. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. WalkthroughThis PR adds a comprehensive suite of unit tests across the v3/pkg/application package covering options, dialogs, menus, context, keyboard accelerators, services, screen management, single-instance handling, and related constants (no production API changes). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
v3/pkg/application/dialogs_test.go (1)
31-39: Consider strengthening the TestFreeDialogID test.The test only verifies that
freeDialogIDdoesn't panic. Consider adding assertions to verify that the freed ID is actually available for reuse by callinggetDialogID()after freeing and checking if the same ID is returned.v3/pkg/application/keys_test.go (1)
171-187: Consider deep cloning for accelerator.Modifiers slice.Lines 185-186 note that the clone is shallow and modifying the clone's modifiers slice would affect the original. If immutability is desired, consider implementing a deep clone that copies the Modifiers slice.
Context
If accelerators are intended to be immutable after creation, a shallow clone defeats that purpose. However, if the current behavior is intentional, the comment is sufficient documentation.
v3/pkg/application/context_test.go (1)
91-100: Consider whether testing internal implementation details is necessary.Lines 93-94 directly manipulate the internal
datamap to test type assertion failure. While this ensures robustness, it couples the test to the internal implementation. If the internal storage mechanism changes, this test would need updating.This is acceptable for now, but consider whether there's a public API way to reach this code path, or if this edge case is unlikely enough to not warrant testing.
v3/pkg/application/application_options_test.go (1)
9-19: Consider the value of testing iota-based constant values.While testing explicit constant values provides documentation, iota-based enum values are compiler-guaranteed and won't accidentally change. This test is acceptable but could be considered optional since the values are structurally guaranteed by Go's iota mechanism.
v3/pkg/application/menuitem_internal_test.go (1)
7-24: Consider the value of testing iota-based type constants.Similar to the ActivationPolicy test, testing iota enum values is pedantic but acceptable for documentation purposes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
REVIEW.mdv3/UNRELEASED_CHANGELOG.mdv3/pkg/application/application_options_test.gov3/pkg/application/context_test.gov3/pkg/application/dialogs_test.gov3/pkg/application/keys_test.gov3/pkg/application/menu_internal_test.gov3/pkg/application/menuitem_internal_test.gov3/pkg/application/parameter_test.gov3/pkg/application/screenmanager_internal_test.gov3/pkg/application/services_test.gov3/pkg/application/single_instance_test.gov3/pkg/application/webview_window_options_test.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-02-17T14:27:22.612Z
Learnt from: fbbdev
Repo: wailsapp/wails PR: 4066
File: v3/pkg/application/internal/tests/services/startup/startup_test.go:88-102
Timestamp: 2025-02-17T14:27:22.612Z
Learning: In test code where mock services are created and controlled within the test environment (e.g., by embedding test interfaces), type assertion safety checks are unnecessary as the interface implementation is guaranteed.
Applied to files:
v3/pkg/application/services_test.go
📚 Learning: 2025-03-24T20:22:56.233Z
Learnt from: popaprozac
Repo: wailsapp/wails PR: 4098
File: v3/pkg/services/notifications/notifications.go:46-55
Timestamp: 2025-03-24T20:22:56.233Z
Learning: In the notifications package, initialization of the `Service` struct is handled through platform-specific `New()` functions in each implementation file (darwin, windows, linux) rather than a generic constructor in the main package file.
Applied to files:
v3/pkg/application/services_test.go
📚 Learning: 2024-09-20T23:34:29.841Z
Learnt from: nixpare
Repo: wailsapp/wails PR: 3763
File: v3/examples/keybindings/main.go:16-17
Timestamp: 2024-09-20T23:34:29.841Z
Learning: In the codebase, `application.Options.KeyBindings` uses the `application.Window` type, whereas `application.WebviewWindowOptions.KeyBindings` uses `*application.WebviewWindow`. This is intentional and acceptable.
Applied to files:
v3/pkg/application/webview_window_options_test.go
📚 Learning: 2025-01-24T22:41:18.566Z
Learnt from: leaanthony
Repo: wailsapp/wails PR: 4031
File: v3/pkg/application/menu.go:199-202
Timestamp: 2025-01-24T22:41:18.566Z
Learning: In the Wails menu system (v3/pkg/application/menu.go), shared state between menus is intentionally designed and desirable. Methods like `Append()` and `Prepend()` should maintain shared references to menu items rather than creating deep copies.
Applied to files:
v3/pkg/application/menu_internal_test.gov3/pkg/application/menuitem_internal_test.go
🧬 Code graph analysis (11)
v3/pkg/application/dialogs_test.go (1)
v3/pkg/application/dialogs.go (4)
InfoDialogType(40-40)QuestionDialogType(41-41)WarningDialogType(42-42)ErrorDialogType(43-43)
v3/pkg/application/services_test.go (1)
v3/pkg/application/services.go (5)
ServiceOptions(17-40)ServiceShutdown(118-120)NewService(48-50)NewServiceWithOptions(55-59)DefaultServiceOptions(44-44)
v3/pkg/application/webview_window_options_test.go (1)
v3/pkg/application/webview_window_options.go (50)
NewRGBPtr(167-172)BackgroundTypeSolid(177-177)BackgroundTypeTransparent(178-178)BackgroundTypeTranslucent(179-179)DragEffectNone(189-189)DragEffectCopy(191-191)DragEffectMove(193-193)DragEffectLink(195-195)MacBackdropNormal(381-381)MacBackdropTransparent(383-383)MacBackdropTranslucent(385-385)MacBackdropLiquidGlass(387-387)MacToolbarStyleAutomatic(395-395)MacToolbarStyleExpanded(397-397)MacToolbarStylePreference(399-399)MacToolbarStyleUnified(401-401)MacToolbarStyleUnifiedCompact(403-403)MacTitleBarDefault(542-549)MacTitleBarHidden(556-563)MacTitleBarHiddenInset(567-574)MacTitleBarHiddenInsetUnified(578-586)MacAppearanceType(589-589)DefaultAppearance(593-593)NSAppearanceNameAqua(595-595)NSAppearanceNameDarkAqua(597-597)NSAppearanceNameVibrantLight(599-599)MacWindowLevel(496-496)MacWindowLevelNormal(499-499)MacWindowLevelFloating(500-500)MacWindowLevelTornOffMenu(501-501)MacWindowLevelModalPanel(502-502)MacWindowLevelMainMenu(503-503)MacWindowLevelStatus(504-504)MacWindowLevelPopUpMenu(505-505)MacWindowLevelScreenSaver(506-506)WebviewWindowOptions(32-143)MacWindow(468-494)CoreWebView2PermissionKindUnknownPermission(211-211)CoreWebView2PermissionKindMicrophone(212-212)CoreWebView2PermissionKindCamera(213-213)CoreWebView2PermissionStateDefault(223-223)CoreWebView2PermissionStateAllow(224-224)CoreWebView2PermissionStateDeny(225-225)LiquidGlassStyleAutomatic(411-411)LiquidGlassStyleLight(413-413)LiquidGlassStyleDark(415-415)LiquidGlassStyleVibrant(417-417)NSVisualEffectMaterialAppearanceBased(425-425)NSVisualEffectMaterialLight(426-426)NSVisualEffectMaterialAuto(442-442)
v3/pkg/application/menu_internal_test.go (3)
v3/pkg/application/menuitem.go (1)
NewMenuItem(70-78)v3/pkg/application/menu.go (1)
NewSubmenu(229-233)v3/pkg/application/messageprocessor_contextmenu.go (1)
ContextMenuData(7-12)
v3/pkg/application/parameter_test.go (2)
v3/pkg/application/bindings.go (5)
Parameter(42-46)CallError(31-35)ReferenceError(26-26)ErrorKind(23-23)TypeError(27-27)v3/pkg/errs/utils.go (1)
Cause(20-40)
v3/pkg/application/keys_test.go (2)
v3/pkg/application/keys.go (1)
SuperKey(22-22)v2/pkg/menu/keys/keys.go (1)
Key(49-53)
v3/pkg/application/application_options_test.go (1)
v3/pkg/application/application_options.go (21)
ActivationPolicyRegular(185-185)ActivationPolicyAccessory(188-188)ActivationPolicyProhibited(189-189)NativeTabIcon(319-319)NativeTabIconNone(323-323)NativeTabIconHouse(324-324)NativeTabIconGear(325-325)NativeTabIconStar(326-326)NativeTabIconPerson(327-327)NativeTabIconBell(328-328)NativeTabIconMagnify(329-329)NativeTabIconList(330-330)NativeTabIconFolder(331-331)MacOptions(193-199)ActivationPolicy(181-181)WindowsOptions(204-225)LinuxOptions(230-242)IOSOptions(247-300)AndroidOptions(337-355)AssetOptions(126-145)NativeTabItem(305-308)
v3/pkg/application/single_instance_test.go (2)
v3/pkg/application/messageprocessor_args.go (1)
Args(8-10)v3/pkg/application/single_instance.go (1)
SingleInstanceOptions(27-45)
v3/pkg/application/context_test.go (1)
v3/pkg/application/messageprocessor_contextmenu.go (1)
ContextMenuData(7-12)
v3/pkg/application/menuitem_internal_test.go (2)
v3/pkg/application/menuitem.go (5)
NewMenuItem(70-78)NewMenuItemSeparator(80-86)NewMenuItemCheckbox(88-97)NewMenuItemRadio(99-108)NewSubMenuItem(110-121)v3/pkg/application/context.go (1)
Context(3-6)
v3/pkg/application/screenmanager_internal_test.go (1)
v3/pkg/application/screenmanager.go (8)
TOP(60-60)RIGHT(61-61)BOTTOM(62-62)LEFT(63-63)BEGIN(67-67)END(68-68)Point(47-50)ScreenPlacement(83-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Cloudflare Pages
🔇 Additional comments (26)
v3/UNRELEASED_CHANGELOG.md (1)
20-23: LGTM! Clear documentation of test additions.The changelog entry clearly documents the comprehensive test coverage improvements, including specific areas tested and the coverage increase achieved.
v3/pkg/application/screenmanager_internal_test.go (2)
141-206: Excellent test coverage for rectangle intersection logic.The table-driven test covers all important scenarios: overlapping, no overlap (horizontal/vertical), containment, identical rectangles, empty rectangles, and touching edges. This comprehensive approach ensures the intersection logic is robust.
263-307: Good coverage of scaling behavior with multiple scale factors.Testing scale operations with both 2.0 and 1.5 scale factors, in both directions (to DIP and to physical), ensures the rounding logic works correctly. This is particularly important for handling fractional scale factors.
v3/pkg/application/single_instance_test.go (2)
9-40: LGTM! Comprehensive encrypt/decrypt round-trip test.The test properly verifies that encrypted data differs from plaintext and that decryption correctly recovers the original message. Using a hardcoded key is appropriate for deterministic unit tests.
199-209: Good nil safety coverage.Testing that cleanup operations don't panic with nil receivers or nil fields is essential for preventing runtime crashes in edge cases. This defensive approach improves robustness.
v3/pkg/application/webview_window_options_test.go (3)
41-53: Good documentation of packed color format.The comment on line 48 clearly explains the
0x00BBGGRRpacked format, making the test expectations understandable. This is helpful for anyone maintaining this code.
157-235: Comprehensive validation of Mac title bar presets.Testing all four Mac title bar presets (Default, Hidden, HiddenInset, HiddenInsetUnified) with field-by-field validation ensures these commonly-used configurations remain correct. This prevents subtle UI bugs from creeping in.
378-388: Important edge case: negative constant value.Testing that
NSVisualEffectMaterialAutoequals-1is crucial since negative enum values are unusual and could be easily broken by refactoring. This test protects against accidental changes.v3/pkg/application/context_test.go (1)
7-120: LGTM! Comprehensive context data management tests.The tests thoroughly cover context initialization, data storage/retrieval, edge cases (nil values, wrong types), and fluent API chaining behavior. This provides good confidence in the context implementation.
v3/pkg/application/application_options_test.go (4)
21-43: LGTM! Effective use of table-driven tests.The table-driven approach cleanly validates all NativeTabIcon constant values.
45-147: LGTM! Thorough middleware testing.The middleware tests comprehensively cover empty, single, and multiple middleware scenarios with proper call order verification.
149-257: LGTM! Default value tests provide useful documentation.While testing zero values might seem pedantic, these tests serve as living documentation of expected defaults and will catch unintended changes to default behavior.
259-301: LGTM! Good coverage of struct fields and short-circuit behavior.The NativeTabItem test and middleware short-circuit test properly validate expected behaviors.
v3/pkg/application/menu_internal_test.go (4)
7-117: LGTM! Solid foundational menu tests with proper cleanup.The basic menu operation tests are well-structured and consistently use
menu.Destroy()for cleanup, preventing test pollution.
119-231: LGTM! Comprehensive edge case coverage for menu item access.The tests properly validate bounds checking, search functionality, and nested submenu traversal.
233-303: LGTM! Menu composition tests include good nil-safety checks.The tests properly validate Append, Prepend, and Clone operations, including the important nil-safety case.
305-403: LGTM! Important validation of radio group logic.The
TestMenu_ProcessRadioGroupstest is particularly valuable as it validates complex radio button grouping behavior with multiple groups separated by non-radio items. Based on learnings, shared state between menu items is intentional in the Wails menu system.v3/pkg/application/menuitem_internal_test.go (4)
26-129: LGTM! Thorough factory function tests with proper cleanup.The factory tests comprehensively validate item creation for all menu item types and consistently clean up with
removeMenuItemByID().
131-171: LGTM! Solid validation of map-based storage and ID uniqueness.The tests properly verify CRUD operations and ID generation behavior.
173-309: LGTM! Comprehensive property getter/setter coverage.The tests thoroughly validate all menu item properties including callback behavior.
311-413: LGTM! Good validation of accelerator handling and fluent API.The tests properly verify accelerator operations, cloning semantics (including the note about ID preservation), and method chaining.
v3/pkg/application/services_test.go (4)
8-49: LGTM! Clean test service implementations.The mock service types are well-structured and provide clear interfaces for testing service lifecycle behavior.
51-105: LGTM! Excellent validation of service naming precedence.The three naming tests (FromOptions, FromInterface, FromType) comprehensively verify the documented priority: options > interface > type name. Line 101 correctly expects the fully qualified type name.
107-165: LGTM! Good coverage of service instance and options handling.The tests properly verify instance retrieval, default options, and custom MarshalError behavior. The type assertion at line 117 is safe in this test-controlled context.
167-203: LGTM! Proper validation of lifecycle interfaces.The ServiceStartup and ServiceShutdown tests correctly verify interface implementation and invocation. Type assertions at lines 173 and 192 are safe in this test-controlled context, as confirmed by retrieved learnings.
REVIEW.md (1)
1-137: LGTM! Comprehensive documentation of testing effort.This document provides excellent context for the PR, including:
- Clear breakdown of what each test file covers
- Honest assessment of coverage limitations due to platform-specific code
- Practical recommendations for future improvements
The documentation will be valuable for future contributors understanding the test landscape.
Deploying wails with
|
| Latest commit: |
e8afd7c
|
| Status: | ⚡️ Build in progress... |
- dialogs_test.go: improve ID recycling test to verify either recycled ID (id3 == id1) or new unique ID (id3 > id2) - keys_test.go: make accelerator String() tests platform-agnostic by checking suffix patterns rather than exact platform-specific strings 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
v3/pkg/application/dialogs_test.gov3/pkg/application/keys_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/pkg/application/keys_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
v3/pkg/application/dialogs_test.go (3)
v2/pkg/menu/menuitem.go (1)
Label(272-277)v2/pkg/menu/callback.go (1)
Callback(8-8)v3/pkg/application/dialogs.go (4)
InfoDialogType(40-40)QuestionDialogType(41-41)WarningDialogType(42-42)ErrorDialogType(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
v3/pkg/application/dialogs_test.go (2)
7-33: LGTM! ID recycling test properly addresses previous feedback.The updated assertion logic correctly validates both recycling and sequential ID allocation scenarios. The test now properly accepts either outcome (recycled
id1or new unique ID greater thanid2) as valid behavior.
45-223: LGTM! Comprehensive test coverage for Button methods and dialog options.The remaining tests provide solid coverage:
- Button method chaining and callback behavior (lines 45-114)
- DialogType constant uniqueness (lines 116-127)
- Field initialization for all dialog option structs (lines 129-223)
All assertions are appropriate and the tests follow consistent patterns for validating struct field assignments and method chaining.
| func TestFreeDialogID(t *testing.T) { | ||
| id := getDialogID() | ||
| freeDialogID(id) | ||
|
|
||
| // Should be able to get the same ID again after freeing | ||
| newID := getDialogID() | ||
| freeDialogID(newID) | ||
| // Just verify it doesn't panic | ||
| } |
There was a problem hiding this comment.
Clarify the test's actual scope.
The comment on line 39 suggests the test verifies ID reuse, but the test only checks that freeDialogID and subsequent getDialogID calls don't panic. ID reuse is already tested in TestGetDialogID. Consider updating the comment to reflect the actual test scope.
🔎 Suggested clarification
- // Should be able to get the same ID again after freeing
newID := getDialogID()
freeDialogID(newID)
- // Just verify it doesn't panic
+ // Verify freeing and subsequent allocation don't panic📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestFreeDialogID(t *testing.T) { | |
| id := getDialogID() | |
| freeDialogID(id) | |
| // Should be able to get the same ID again after freeing | |
| newID := getDialogID() | |
| freeDialogID(newID) | |
| // Just verify it doesn't panic | |
| } | |
| func TestFreeDialogID(t *testing.T) { | |
| id := getDialogID() | |
| freeDialogID(id) | |
| newID := getDialogID() | |
| freeDialogID(newID) | |
| // Verify freeing and subsequent allocation don't panic | |
| } |
🤖 Prompt for AI Agents
In v3/pkg/application/dialogs_test.go around lines 35 to 43, the inline comment
claims the test verifies ID reuse but the test only ensures that calling
freeDialogID followed by getDialogID does not panic; update the comment to
accurately state that the test checks that freeing an ID and subsequently
obtaining an ID does not cause a panic (or remove the misleading sentence), or
alternatively change the test to assert actual ID reuse if that was
intended—make the comment and test behavior consistent.
os.TempDir() returns a trailing slash on macOS, causing path comparison to fail. Use filepath.Clean() to normalize both paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
v3/UNRELEASED_CHANGELOG.md (1)
20-20: Add PR number reference to enhance traceability.The changelog entry follows the "Keep a Changelog" format and uses appropriate present tense, but the guidelines state to "Reference issue/PR numbers when applicable." Include the PR number for better traceability and cross-reference consistency with other entries in the changelog (e.g., line 27 includes
(#4151)).🔎 Proposed update
-- Add unit tests for pkg/application by @leaanthony +- Add unit tests for pkg/application (#4827) by @leaanthony
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Analyze (go)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Cloudflare Pages
|
|
1 participant
Summary
Test plan
go test ./v3/pkg/application/...Details
See REVIEW.md for full breakdown of new tests and coverage analysis.
🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.