Repository navigation
add tests - #14
add tests#14
Conversation
WalkthroughAdds CI/CD configurations (test, build, security, dependency review, Codecov, Dependabot), introduces Windows build tags and non-Windows stubs, converts internal control flow to context-driven handlers, and adds a large suite of cross-platform and Windows-gated tests plus docs, gitignore, and linting config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Developer
participant GitHub as "GitHub"
participant Test as "Test Suite\n(.github/workflows/test.yml)"
participant Build as "Build & Publish\n(.github/workflows/build-and-publish.yml)"
Developer->>GitHub: push / open PR
GitHub->>Test: run Test Suite (fmt, lint, tests, coverage + Codecov)
Test-->>GitHub: status + coverage artifact
alt tests pass
GitHub->>Build: workflow_call -> build-and-publish
Build->>Build: cache deps, go mod download + go mod verify, build artifacts
Build->>Build: verify artifacts, gen SHA256, upload release assets (VT scan if key)
Build-->>GitHub: release artifacts published
else tests fail
Test-->>GitHub: fail status, stop build
end
GitHub->>GitHub: Security Scan (scheduled/PR) runs CodeQL, govulncheck, staticcheck
GitHub->>GitHub: Dependency Review runs + license report uploaded
sequenceDiagram
autonumber
participant App as "App (main)"
participant Ev as "startEventHandlersWithContext"
participant Menu as "handleMenuItemClicksWithContext"
participant Upd as "checkForUpdateWithContext / startUpdateClickHandlerWithContext"
Developer->>App: start()
App->>Ev: create context.Context
Ev->>Menu: spawn menu handler goroutine with ctx
Ev->>Upd: spawn update checker/handler goroutines with ctx
Note right of Menu: menu loop listens for item clicks and ctx.Done()
Note right of Upd: update loop polls/handles clicks and ctx.Done()
Developer->>App: cancel context / shutdown
App->>Menu: ctx.Done() -> exit loop
App->>Upd: ctx.Done() -> exit loop
Menu-->>App: cleanup
Upd-->>App: cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF ScorecardScorecard details
Scanned Files
|
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
![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 (9)
.github/workflows/test.yml (1)
162-165: Remove deprecated action inputs.The
skip-pkg-cacheandskip-build-cacheinputs are not supported ingolangci-lint-action@v6. v5.0.0+ removes skip-pkg-cache and skip-build-cache because the cache related to Go itself is already handled by actions/setup-go.Apply this diff to use the correct inputs:
- skip-cache: false - skip-pkg-cache: false - skip-build-cache: false + skip-cache: false + skip-save-cache: falseutils_test.go (1)
25-41: Improve cross-platform test robustness.The test skips Windows but doesn't verify the intended behavior. Consider adding assertions to check that
openBrowserhandles non-Windows platforms appropriately (e.g., by checking if an error is logged or a specific behavior occurs).updater_logic_test.go (1)
64-73: Improve test assertion logic.The test logic
strings.Contains("winget install "+expectedPackageID, expectedPackageID)will always returntruesince the expected package ID is explicitly included in the constructed string. Consider testing actual command generation or refactor to validate the real update command components.Apply this diff to improve the test:
- // This tests the knowledge that the update uses winget - if !strings.Contains("winget install "+expectedPackageID, expectedPackageID) { - t.Error("Update command should reference correct package ID") - } + // Test the actual command construction logic if available + // or verify that the package ID constant is correct + if expectedPackageID != "5mdt.WinGoDarkTray" { + t.Errorf("Package ID should be '5mdt.WinGoDarkTray', got %s", expectedPackageID) + }pure_funcs_test.go (1)
172-208: Consider using constants for expected UI text values.The hardcoded UI text mappings in the test could become out of sync with the actual application constants. Consider importing and testing against the actual constants from the main codebase to ensure consistency.
ui_texts_test.go (5)
69-80: Make content checks case‑insensitive to reduce brittleness.Current assertions will fail if copy changes only differ by case (e.g., “windows” vs “Windows”). Use case‑insensitive containment.
Apply this diff:
- if !strings.Contains(menuTitles.ToggleWinToDark, "Windows") { + if !strings.Contains(strings.ToLower(menuTitles.ToggleWinToDark), "windows") { t.Error("ToggleWinToDark should contain 'Windows'") } - if !strings.Contains(menuTitles.ToggleWinToLight, "Windows") { + if !strings.Contains(strings.ToLower(menuTitles.ToggleWinToLight), "windows") { t.Error("ToggleWinToLight should contain 'Windows'") } - if !strings.Contains(menuTitles.ToggleBothToDark, "both") { + if !strings.Contains(strings.ToLower(menuTitles.ToggleBothToDark), "both") { t.Error("ToggleBothToDark should contain 'both'") } - if !strings.Contains(menuTitles.ToggleBothToLight, "both") { + if !strings.Contains(strings.ToLower(menuTitles.ToggleBothToLight), "both") { t.Error("ToggleBothToLight should contain 'both'") }- if !strings.Contains(tooltips.Default, "windows") || !strings.Contains(tooltips.Default, "toggle") { + if !strings.Contains(strings.ToLower(tooltips.Default), "windows") || !strings.Contains(strings.ToLower(tooltips.Default), "toggle") { t.Error("Default tooltip should describe the app's purpose") }Also applies to: 93-96
8-46: Reduce duplication with a table‑driven structure test.This keeps the file concise and improves failure diagnostics; also trims accidental whitespace.
-func TestMenuTitlesStructure(t *testing.T) { - // Test that all menu titles are non-empty - if menuTitles.AppName == "" { - t.Error("AppName should not be empty") - } - if menuTitles.EnableAutorun == "" { - t.Error("EnableAutorun should not be empty") - } - if menuTitles.Quit == "" { - t.Error("Quit should not be empty") - } - if menuTitles.EnableAutorunChecked == "" { - t.Error("EnableAutorunChecked should not be empty") - } - if menuTitles.EnableAutorunUnchecked == "" { - t.Error("EnableAutorunUnchecked should not be empty") - } - if menuTitles.ToggleAppToDark == "" { - t.Error("ToggleAppToDark should not be empty") - } - if menuTitles.ToggleAppToLight == "" { - t.Error("ToggleAppToLight should not be empty") - } - if menuTitles.ToggleWinToDark == "" { - t.Error("ToggleWinToDark should not be empty") - } - if menuTitles.ToggleWinToLight == "" { - t.Error("ToggleWinToLight should not be empty") - } - if menuTitles.ToggleBothToDark == "" { - t.Error("ToggleBothToDark should not be empty") - } - if menuTitles.ToggleBothToLight == "" { - t.Error("ToggleBothToLight should not be empty") - } - if menuTitles.UpdateNow == "" { - t.Error("UpdateNow should not be empty") - } -} +func TestMenuTitlesStructure(t *testing.T) { + t.Parallel() + cases := []struct { + name string + val string + }{ + {"AppName", menuTitles.AppName}, + {"EnableAutorun", menuTitles.EnableAutorun}, + {"Quit", menuTitles.Quit}, + {"EnableAutorunChecked", menuTitles.EnableAutorunChecked}, + {"EnableAutorunUnchecked", menuTitles.EnableAutorunUnchecked}, + {"ToggleAppToDark", menuTitles.ToggleAppToDark}, + {"ToggleAppToLight", menuTitles.ToggleAppToLight}, + {"ToggleWinToDark", menuTitles.ToggleWinToDark}, + {"ToggleWinToLight", menuTitles.ToggleWinToLight}, + {"ToggleBothToDark", menuTitles.ToggleBothToDark}, + {"ToggleBothToLight", menuTitles.ToggleBothToLight}, + {"UpdateNow", menuTitles.UpdateNow}, + } + for _, c := range cases { + if strings.TrimSpace(c.val) == "" { + t.Errorf("%s should not be empty", c.name) + } + } +}
48-48: Parallelize independent tests.These tests only read constants/globals; safe to run in parallel for faster CI.
func TestMenuTitlesContent(t *testing.T) { + t.Parallel()
83-83: Parallelize independent tests.func TestTooltipsStructure(t *testing.T) { + t.Parallel()
98-98: Parallelize independent tests.func TestNotificationTextsStructure(t *testing.T) { + t.Parallel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.codecov.yml(1 hunks).github/dependabot.yml(1 hunks).github/workflows/build-and-publish.yml(4 hunks).github/workflows/dependency-review.yml(1 hunks).github/workflows/security.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).golangci.yml(1 hunks).pre-commit-config.yaml(0 hunks)README.md(1 hunks)app_test.go(1 hunks)cross_platform_test.go(1 hunks)pure_funcs_test.go(1 hunks)semver_utils_test.go(1 hunks)testable_funcs.go(1 hunks)ui_texts_test.go(1 hunks)updater_logic_test.go(1 hunks)utils_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
app_test.go (1)
main.go (1)
NewApp(19-26)
🪛 actionlint (1.7.7)
.github/workflows/test.yml
163-163: input "skip-pkg-cache" is not defined in action "golangci/golangci-lint-action@v6". available inputs are "args", "cache-invalidation-interval", "github-token", "install-mode", "only-new-issues", "problem-matchers", "skip-cache", "skip-save-cache", "version", "working-directory"
(action)
164-164: input "skip-build-cache" is not defined in action "golangci/golangci-lint-action@v6". available inputs are "args", "cache-invalidation-interval", "github-token", "install-mode", "only-new-issues", "problem-matchers", "skip-cache", "skip-save-cache", "version", "working-directory"
(action)
.github/workflows/build-and-publish.yml
90-90: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🪛 GitHub Check: Go Vulnerability Check
testable_funcs.go
[failure] 28-28:
setTemporaryTooltip redeclared in this block
[failure] 24-24:
logEvent redeclared in this block
[failure] 20-20:
showError redeclared in this block
🔇 Additional comments (18)
README.md (1)
4-9: LGTM! Good addition of status badges.The badges provide valuable visibility into the project's CI/CD status, test coverage, and code quality. They align well with the new workflows and tooling introduced in this PR.
.gitignore (1)
1-8: LGTM! Appropriate gitignore strategy for development tooling.The global dotfile ignore with specific exceptions is a clean approach that prevents accidentally tracking IDE configuration files while ensuring essential development tooling (CI/CD, linting, coverage) is version controlled.
.codecov.yml (1)
1-44: LGTM! Comprehensive Codecov configuration with appropriate settings.The configuration establishes reasonable coverage thresholds (70%), proper CI integration, and logical ignore patterns. The Windows-specific flag and path configuration aligns well with the project's platform focus.
.github/dependabot.yml (1)
8-24: LGTM! Well-configured dependency management.The weekly schedule, appropriate reviewers, and reasonable PR limits (5 for Go, 3 for GitHub Actions) provide good automation without overwhelming maintainers.
semver_utils_test.go (2)
8-55: LGTM! Comprehensive test coverage for version utilities.The table-driven tests cover all edge cases including padding scenarios, empty inputs, and boundary conditions. The test structure follows Go testing best practices.
57-104: LGTM! Thorough testing of the max function.Good coverage of edge cases including negative numbers and equality scenarios. The tests are well-structured and follow consistent patterns.
testable_funcs.go (1)
49-56: LGTM! Appropriate testing factory function.The
NewAppForTestingfunction provides a clean way to create mock app instances for tests with reasonable defaults. The version defaulting to "v0.0.0" when empty is appropriate for testing scenarios..golangci.yml (2)
24-41: LGTM! Comprehensive linter configuration with appropriate exceptions.The enabled linters provide good coverage of common Go issues. The disabled linters make sense -
gosecfor Windows registry operations andmalignedfor struct alignment being non-critical.
47-66: LGTM! Well-thought exclusion rules for different contexts.The exclusions are appropriate:
- Test files should have relaxed linting rules
- Windows-specific code exclusions from cross-platform import checks
- Long lines allowed in tests for readability
These rules support the cross-platform testing approach introduced in this PR.
cross_platform_test.go (2)
12-45: LGTM! Comprehensive version comparison testing.The test cases cover edge cases including mixed version formats, invalid versions, and various comparison scenarios. The cross-platform build constraints ensure these tests run on non-Windows systems where the actual Windows APIs aren't available.
47-69: Duplicate tests — consolidate or justify.TestMaxCrossPlatform in cross_platform_test.go (lines 47–69) is identical to TestMax in semver_utils_test.go (lines 57–104). Keep a single test or add explicit build tags / different intent to justify both.
.github/workflows/dependency-review.yml (1)
50-50: LGTM: Well-configured dependency and license scanning.This workflow provides comprehensive dependency and license review with appropriate configurations. The use of the
|| trueflag in the license report generation is wise to prevent failures when no licenses are detected..github/workflows/build-and-publish.yml (1)
30-30: Verify Go version compatibility.Go version '1.25' is specified, but this version may not exist yet. Please verify the available Go versions and consider using a stable version like '1.23'.
.github/workflows/security.yml (1)
1-103: LGTM: Comprehensive security scanning setup.This workflow provides excellent security coverage with multiple scanning tools (govulncheck, CodeQL, Nancy, and Gosec). The configuration includes proper permissions, scheduled runs, and SARIF uploads for integration with GitHub Security features.
app_test.go (1)
30-55: LGTM: Thorough App initialization testing.The test covers various version scenarios and validates proper initialization of menu items. The table-driven approach is well-structured and comprehensive.
pure_funcs_test.go (1)
12-81: LGTM: Well-designed pure function tests.The version comparison logic is thoroughly tested with edge cases, and the implementation handles version padding correctly. The test coverage includes both valid and invalid inputs.
ui_texts_test.go (2)
48-81: Overall: good coverage and intent checks.Nice mix of structure and content assertions; naming is clear and failures will be actionable.
Also applies to: 98-117
1-1: Resolve — non-Windows stubs present; no Windows-only build tag requiredui_texts.go defines menuTitles, tooltips, and notificationTexts (see ui_texts.go:3,31,39), so ui_texts_test.go will compile cross-platform.
| func showError(message string) { | ||
| fmt.Printf("Error: %s\n", message) | ||
| } | ||
|
|
||
| func logEvent(level interface{}, message string) { | ||
| fmt.Printf("Log: %s\n", message) | ||
| } | ||
|
|
||
| func setTemporaryTooltip(message string, duration interface{}) { | ||
| fmt.Printf("Tooltip: %s\n", message) | ||
| } | ||
|
|
||
| func openRegistryKey(path string, access uint32) (interface{}, error) { | ||
| return mockRegistryKey{}, errors.New("registry not available on non-Windows") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Fix function redeclaration errors.
The static analysis correctly identifies that these functions are being redeclared. These stub implementations conflict with the actual Windows implementations when both are compiled together.
Based on the search results, the issue is clear. No identifier may be declared twice in the same block, and no identifier may be declared in both the file and package block. The build tags are working correctly - the problem is that these functions already exist in the main Windows implementation files, and static analysis tools are seeing both versions simultaneously.
Let me verify if these functions exist in other files:
🏁 Script executed:
#!/bin/bash
# Description: Check if these stub functions are already implemented in Windows-specific files
echo "Searching for Windows-specific function implementations..."
rg -n --type=go -C3 "func showError|func logEvent|func setTemporaryTooltip|func openRegistryKey" -g '!testable_funcs.go'Length of output: 207
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "---- show testable_funcs.go (first 120 lines) ----"
if [ -f testable_funcs.go ]; then
sed -n '1,120p' testable_funcs.go || true
else
echo "testable_funcs.go not found at repo root"
fi
echo
echo "---- list tracked .go files (first 200) ----"
git ls-files '*.go' | sed -n '1,200p' || true
echo
echo "---- search for function DEFINITIONS in other .go files (exclude testable_funcs.go) ----"
OTHER_FILES=$(git ls-files '*.go' | grep -v '^testable_funcs.go$' || true)
if [ -z "$OTHER_FILES" ]; then
echo "No other tracked .go files found"
else
echo "$OTHER_FILES" | sed -n '1,200p'
rg -n --hidden --no-ignore -S "func\s+(showError|logEvent|setTemporaryTooltip|openRegistryKey)\s*\(" $(echo $OTHER_FILES) -C3 || true
fi
echo
echo "---- search in windows-specific files (*_windows.go) for DEFINITIONS ----"
rg -n --hidden --no-ignore -S "func\s+(showError|logEvent|setTemporaryTooltip|openRegistryKey)\s*\(" -g '*_windows.go' -C3 || true
echo
echo "---- fallback: search entire repo for any occurrences of these identifiers ----"
rg -n --hidden --no-ignore -S "(showError|logEvent|setTemporaryTooltip|openRegistryKey)" -C3 || trueLength of output: 13455
Fix function redeclaration errors.
- testable_funcs.go (//go:build !windows) provides stubs: showError, logEvent, setTemporaryTooltip, openRegistryKey.
- Duplicates found in: logging.go (logEvent ≈ line 71, showError ≈ line 111, setTemporaryTooltip ≈ line 124) and utils.go (openRegistryKey ≈ line 23).
- Action: ensure only one implementation is compiled per GOOS — e.g. mark Windows-only files with //go:build windows or move Windows-specific functions into *_windows.go, or remove/rename the non-Windows stubs so the package no longer redeclares these identifiers.
🧰 Tools
🪛 GitHub Check: Go Vulnerability Check
[failure] 28-28:
setTemporaryTooltip redeclared in this block
[failure] 24-24:
logEvent redeclared in this block
[failure] 20-20:
showError redeclared in this block
🤖 Prompt for AI Agents
In testable_funcs.go (lines 20-34) you have non-Windows stubs that conflict with
Windows implementations in logging.go and utils.go; to resolve redeclaration
errors, ensure only one implementation is built per GOOS: move or rename the
Windows-specific functions into files with a //go:build windows (or
*_windows.go) tag (or add that build tag to the existing Windows files), or
conversely remove/rename the non-Windows stubs in testable_funcs.go so the
identifiers are not duplicated; pick one approach and make the Windows-only
implementations live in windows-tagged files while keeping the non-Windows stubs
in files with //go:build !windows.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tray_handlers.go (1)
10-25: Stop the goroutine after Quit to avoid spinning on closed channels.
When systray.Quit() is invoked, ClickedCh may close; a select on a closed channel can hot-loop. Return immediately.Apply:
case <-quitItem.ClickedCh: - systray.Quit() + systray.Quit() + return
♻️ Duplicate comments (1)
.github/workflows/build-and-publish.yml (1)
89-98: Fix secrets context usage in if condition (repeat).secrets is not allowed in step-level if. Export to env and check env.VT_API_KEY.
- - name: VirusTotal Scan - if: ${{ secrets.VT_API_KEY != '' }} + - name: VirusTotal Scan + if: env.VT_API_KEY != '' uses: crazy-max/ghaction-virustotal@v4 + env: + VT_API_KEY: ${{ secrets.VT_API_KEY }} with: - vt_api_key: ${{ secrets.VT_API_KEY }} + vt_api_key: ${{ env.VT_API_KEY }} files: | ./build/WinGoDarkTray-x32.exe ./build/WinGoDarkTray-x64.exe ./build/WinGoDarkTray-arm64.exe
🧹 Nitpick comments (29)
README.md (1)
78-97: Remove duplicate "Building from Source" section.There are two identical "Building from Source" sections (lines 58-77 and 78-97). The duplicate should be removed to avoid confusion.
Apply this diff to remove the duplicate section:
-### Building from Source - -To build WinGoDarkTray from source, follow these steps: - -1. Make sure Go 1.24+ is installed. You can download Go from [here](https://golang.org/dl/). -2. Clone this repository: - - ```bash - git clone https://github.com/5mdt/WinGoDarkTray.git - cd WinGoDarkTray - ``` - -3. Build the application: - - ```bash - go build -o WinGoDarkTray.exe - ``` - -4. Double-click `WinGoDarkTray.exe` to launch it, and it will appear in your system tray. -5. (Optional) Press tray icon and enable autorun.autorun.go (1)
1-5: Consider modernizing build constraints syntax.The go command now understands //go:build lines and prefers them over // +build lines since Go 1.17. The new syntax uses boolean expressions, just like Go, and should be less error-prone. While both syntaxes work, the newer
//go:buildsyntax is preferred and more explicit.Consider using only the modern syntax for consistency across the codebase:
// autorun.go //go:build windows -// +build windowsApply this pattern to all files with Windows build constraints in this PR (autorun.go, logging.go, updater.go, etc.).
logging.go (1)
1-5: Consider modernizing build constraints syntax.Same as autorun.go - consider using only the modern
//go:build windowssyntax without the legacy// +build windowsline for better maintainability and alignment with current Go best practices.// logging.go //go:build windows -// +build windowsupdater.go (2)
1-5: Consider modernizing build constraints syntax.Same as other Windows-gated files - consider using only the modern
//go:build windowssyntax for consistency.// updater.go //go:build windows -// +build windows
81-81: Simplify the update command construction.The current command construction is overly complex with nested cmd commands. Consider breaking this into parts for better readability and maintainability.
func runUpdateCommand() { - cmd := exec.Command("cmd", "/C", "start", "cmd", "/K", "echo Trying to update WinGoDarkTray, using winget app && winget install 5mdt.WinGoDarkTray && echo If nothing installed, please try again later. Winget repository moderators must approve new package first.") + updateScript := "echo Trying to update WinGoDarkTray, using winget app && winget install 5mdt.WinGoDarkTray && echo If nothing installed, please try again later. Winget repository moderators must approve new package first." + cmd := exec.Command("cmd", "/C", "start", "cmd", "/K", updateScript) err := cmd.Start() if err != nil { showError("Failed to run update command: " + err.Error()) } }theme.go (2)
42-57: Reduce duplication and make mode intent explicit.
Minor readability: extract a small helper and name the DWORDs.Apply:
+const ( + modeDark uint32 = 0 + modeLight uint32 = 1 +) + func setBothThemeModes(lightMode bool) error { - var newMode uint32 - if lightMode { - newMode = 1 - } else { - newMode = 0 - } + newMode := modeDark + if lightMode { + newMode = modeLight + }
116-152: Title updates look correct; guard against nil menu items if called earlier.
Currently called after creation, but if this gets reused elsewhere, a nil guard would prevent panics.Apply:
func (a *App) updateThemeToggleTitles() { + if a.toggleAppItem == nil || a.toggleSystemItem == nil || a.toggleWindowsItem == nil { + return + }tray_handlers.go (1)
10-25: Avoid duplicate Quit handling across goroutines.
You’re calling systray.Quit() here and also in startEventHandlers. Prefer a single place and signal via a quit channel.Proposed direction:
- Remove the Quit case here and handle Quit only in startEventHandlers, or
- Pass a quitCh into handleMenuItemClicks and select on it to return.
utils.go (1)
16-26: Windows guard is redundant inside a Windows-gated file; validate scheme instead.
Since this file only builds on Windows, the GOOS check can be dropped. Also, consider allowing only http/https to avoid odd schemes.Apply:
func openBrowser(urlStr string) { - if runtime.GOOS != "windows" { - showError("Opening URLs is only supported on Windows.") - return - } - - err := exec.Command("rundll32", "url.dll,FileProtocolHandler", urlStr).Start() + // optional: basic scheme guard + if !strings.HasPrefix(strings.ToLower(urlStr), "http://") && !strings.HasPrefix(strings.ToLower(urlStr), "https://") { + showError("Only http(s) URLs are supported.") + return + } + err := exec.Command("rundll32", "url.dll,FileProtocolHandler", urlStr).Start() if err != nil { showError("Failed to open browser: " + err.Error()) } }And add:
import ( "os" "os/exec" - "runtime" + "strings"main.go (1)
97-108: Duplicate Quit handling and goroutine lifecycle.
You both listen for quitItem.ClickedCh here and in handleMenuItemClicks. Consolidate and ensure all goroutines exit when quitting.Option:
- Handle Quit only here; pass a quitCh into handleMenuItemClicks and return there on <-quitCh.
app_test.go (1)
32-57: Optional: run subtests in parallel.
Safe here and speeds up CI a bit.Apply:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel().golangci.yml (3)
43-46: Drop maligned; use fieldalignment (vet) if needed.maligned is archived. If you want struct alignment checks, prefer govet’s fieldalignment (already covered indirectly).
disable: - - gosec # Windows registry operations trigger security warnings - - maligned # Struct alignment not critical for this app + - gosec # Windows registry operations trigger security warnings
5-7: Revisit build-tags: windows; it doesn’t switch GOOS.On Ubuntu runners, this won’t include //go:build windows files. Either run golangci-lint on windows-latest or drop this to lint the Linux build.
Would you like me to add a parallel lint job on windows-latest?
49-56: Clean up exclude rules for linters not enabled.You exclude dupl and lll in tests, but they’re not enabled. Either enable them or remove these excludes to reduce config noise.
exclude-rules: # Exclude some linters from running on tests files - path: _test\.go linters: - gocyclo - errcheck - - dupl - - gosec + - gosec @@ # Allow long lines in test files - path: _test\.go linters: - - lll + # - lllAlso applies to: 63-67
updater_logic_test.go (2)
46-64: Stronger URL validation via parsing vs substring checks.Use url.Parse to validate scheme/host/path deterministically.
import ( "strings" "testing" + "net/url" ) @@ func TestGithubAPIURL(t *testing.T) { expectedURL := "https://api.github.com/repos/5mdt/WinGoDarkTray/releases/latest" if githubReleasesAPI != expectedURL { t.Errorf("githubReleasesAPI = %q, want %q", githubReleasesAPI, expectedURL) } - // Verify URL is well-formed - if !strings.HasPrefix(githubReleasesAPI, "https://") { - t.Error("githubReleasesAPI should use HTTPS") - } - - if !strings.Contains(githubReleasesAPI, "github.com") { - t.Error("githubReleasesAPI should point to GitHub") - } - - if !strings.Contains(githubReleasesAPI, "5mdt/WinGoDarkTray") { - t.Error("githubReleasesAPI should point to correct repository") - } + u, err := url.Parse(githubReleasesAPI) + if err != nil { + t.Fatalf("invalid URL: %v", err) + } + if u.Scheme != "https" { + t.Error("githubReleasesAPI should use HTTPS") + } + if u.Host != "api.github.com" || !strings.HasSuffix(u.Path, "/repos/5mdt/WinGoDarkTray/releases/latest") { + t.Error("githubReleasesAPI should point to the correct GitHub API path") + } }
66-75: Test is vacuous; it asserts a literal contains its own substring.Replace with a test of real code (e.g., function/const that builds the winget command) or remove.
If you expose a helper like buildWingetInstallCmd(packageID), I can wire a meaningful test.
.github/workflows/security.yml (1)
76-80: Pin Nancy action to a tag or SHA; avoid @main.Using @main risks breaking on upstream changes. Pin to a stable tag or commit SHA.
- uses: sonatypecommunity/nancy-github-action@main + uses: sonatypecommunity/nancy-github-action@v1ui_texts_test.go (1)
5-8: Make content checks case-insensitive to avoid fragile failures.UI strings may vary in case (“Windows”, “Both”, etc.). Normalize during tests.
import ( "strings" "testing" ) +func containsCI(s, sub string) bool { return strings.Contains(strings.ToLower(s), strings.ToLower(sub)) } @@ - if !strings.Contains(menuTitles.AppName, "WinGoDarkTray") { + if !containsCI(menuTitles.AppName, "WinGoDarkTray") { t.Error("AppName should contain 'WinGoDarkTray'") } @@ - if !strings.Contains(menuTitles.ToggleAppToDark, "Dark") { + if !containsCI(menuTitles.ToggleAppToDark, "Dark") { t.Error("ToggleAppToDark should contain 'Dark'") } - if !strings.Contains(menuTitles.ToggleAppToLight, "Light") { + if !containsCI(menuTitles.ToggleAppToLight, "Light") { t.Error("ToggleAppToLight should contain 'Light'") } - if !strings.Contains(menuTitles.ToggleWinToDark, "Windows") { + if !containsCI(menuTitles.ToggleWinToDark, "Windows") { t.Error("ToggleWinToDark should contain 'Windows'") } - if !strings.Contains(menuTitles.ToggleWinToLight, "Windows") { + if !containsCI(menuTitles.ToggleWinToLight, "Windows") { t.Error("ToggleWinToLight should contain 'Windows'") } - if !strings.Contains(menuTitles.ToggleBothToDark, "both") { + if !containsCI(menuTitles.ToggleBothToDark, "both") { t.Error("ToggleBothToDark should contain 'both'") } - if !strings.Contains(menuTitles.ToggleBothToLight, "both") { + if !containsCI(menuTitles.ToggleBothToLight, "both") { t.Error("ToggleBothToLight should contain 'both'") } @@ - if !strings.Contains(tooltips.Default, "windows") || !strings.Contains(tooltips.Default, "toggle") { + if !containsCI(tooltips.Default, "windows") || !containsCI(tooltips.Default, "toggle") { t.Error("Default tooltip should describe the app's purpose") } @@ - if !strings.Contains(notificationTexts.Error, "WinGoDarkTray") { + if !containsCI(notificationTexts.Error, "WinGoDarkTray") { t.Error("Error notification should contain app name") } - if !strings.Contains(notificationTexts.UpdateAvailableTitle, "Update") { + if !containsCI(notificationTexts.UpdateAvailableTitle, "Update") { t.Error("UpdateAvailableTitle should contain 'Update'") }Also applies to: 50-83, 94-98, 112-119
.github/workflows/build-and-publish.yml (1)
59-75: Artifact names: allow x86/386 variant to avoid false negatives.If build.sh outputs x86/386 instead of x32, the step will fail.
- if [ ! -f "./build/WinGoDarkTray-x32.exe" ]; then - echo "Error: x32 executable not found" - exit 1 - fi + if [ ! -f "./build/WinGoDarkTray-x32.exe" ] && [ ! -f "./build/WinGoDarkTray-x86.exe" ] && [ ! -f "./build/WinGoDarkTray-386.exe" ]; then + echo "Error: 32-bit executable not found (expected one of x32/x86/386)" + exit 1 + fi.github/workflows/dependency-review.yml (1)
47-52: Drop missing template flag from go-licenses commandThere’s no
licenses.tplin the repo, sogo-licenses report --template licenses.tplerrors (silently via|| true) and yields an empty report. Remove the flag:- go-licenses report ./... --template licenses.tpl > licenses-report.txt || true + go-licenses report ./... > licenses-report.txt || trueIf you need custom formatting, add a
licenses.tplfile instead.CLAUDE.md (1)
26-26: Fix Markdown: remove literal "\n\n" and split CI/CD section into its own heading.Current line renders incorrectly; split into separate lines/headings.
Apply this diff:
-**Note**: Full test suite requires Windows environment due to Windows API dependencies. See `TESTING.md` for comprehensive testing strategy.\n\n### CI/CD Workflows\nThe project includes comprehensive GitHub Actions workflows: +**Note**: Full test suite requires Windows environment due to Windows API dependencies. See TESTING.md for comprehensive testing strategy. + +### CI/CD Workflows +The project includes comprehensive GitHub Actions workflows:semver_test.go (1)
7-30: Optional: add a couple more edge cases (leading zeros/whitespace).Strengthens parsing expectations without changing behavior.
Apply this diff to extend the table:
{"v1.2.3", "v1.10.0", false}, {"v1.2.4", "v1.2.3", true}, {"v2.0.0", "v1.9.9", true}, + {"01.02.003", "1.2.3", false}, + {"1.2.3 ", "1.2.3", false},semver_utils_test.go (1)
59-106: Minor: consider t.Parallel in subtests to speed up.Safe here; helpers are pure.
Apply this diff:
func TestMax(t *testing.T) { tests := []struct { @@ for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() got := max(tt.a, tt.b) if got != tt.want { t.Errorf("max(%d, %d) = %d, want %d", tt.a, tt.b, got, tt.want) } }) } }cross_platform_test.go (1)
14-71: Avoid duplication with semver_test.go by sharing test vectors.Extract common version/max test cases into a helper to keep one source of truth.
Example pattern:
-// local tests slice here +// in a shared *_test.go file: +var versionCompareTests = []struct{ latest, current string; want bool }{ + {"0.9.9","0.9.8",true}, /* ... */ +}Then both tests iterate over versionCompareTests.
pure_funcs_test.go (5)
1-11: Prune imports after removing local helpers/UI block.Keep only what’s needed (likely just testing; maybe reflect).
54-73: Optional: dedupe version test cases with semver_test.go.Extract shared cases to avoid drift.
75-83: Optional: t.Parallel inside subtests for speed.Safe for pure comparisons.
Apply:
- t.Run(tt.latest+" vs "+tt.current, func(t *testing.T) { + t.Run(tt.latest+" vs "+tt.current, func(t *testing.T) { + t.Parallel()
133-141: Optional: t.Parallel here too.- t.Run(tt.name, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { + t.Parallel()
164-172: Optional: t.Parallel here too.- t.Run(tt.name, func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { + t.Parallel()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
.codecov.yml(1 hunks).github/dependabot.yml(1 hunks).github/workflows/build-and-publish.yml(4 hunks).github/workflows/dependency-review.yml(1 hunks).github/workflows/security.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).golangci.yml(1 hunks).pre-commit-config.yaml(0 hunks)CLAUDE.md(1 hunks)README.md(1 hunks)TESTING.md(1 hunks)app_test.go(1 hunks)autorun.go(1 hunks)cross_platform_test.go(1 hunks)icon.go(1 hunks)logging.go(1 hunks)main.go(1 hunks)main_stub.go(1 hunks)pure_funcs_test.go(1 hunks)semver.go(1 hunks)semver_test.go(1 hunks)semver_utils_test.go(1 hunks)theme.go(1 hunks)tray_handlers.go(1 hunks)ui_texts.go(1 hunks)ui_texts_test.go(1 hunks)updater.go(1 hunks)updater_logic_test.go(1 hunks)utils.go(1 hunks)utils_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- .pre-commit-config.yaml
🧰 Additional context used
🧬 Code graph analysis (1)
app_test.go (1)
main.go (1)
NewApp(24-31)
🪛 actionlint (1.7.7)
.github/workflows/test.yml
163-163: input "skip-pkg-cache" is not defined in action "golangci/golangci-lint-action@v6". available inputs are "args", "cache-invalidation-interval", "github-token", "install-mode", "only-new-issues", "problem-matchers", "skip-cache", "skip-save-cache", "version", "working-directory"
(action)
164-164: input "skip-build-cache" is not defined in action "golangci/golangci-lint-action@v6". available inputs are "args", "cache-invalidation-interval", "github-token", "install-mode", "only-new-issues", "problem-matchers", "skip-cache", "skip-save-cache", "version", "working-directory"
(action)
.github/workflows/build-and-publish.yml
90-90: context "secrets" is not allowed here. available contexts are "env", "github", "inputs", "job", "matrix", "needs", "runner", "steps", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
⏰ 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). (1)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (21)
semver.go (1)
1-2: LGTM! Header comment follows convention.The header comment addition aligns with the repository-wide convention being applied across all Go files in this PR.
.gitignore (1)
1-9: LGTM! Gitignore changes support CI/CD tooling.The addition of the global dotfile ignore pattern with specific exceptions for CI/CD configuration files (.codecov.yml, .github/, .golangci.yml, .pre-commit-config.yaml) is appropriate for the new workflows and tooling being introduced.
icon.go (1)
1-2: LGTM! Header comment follows convention.The header comment addition aligns with the repository-wide convention being applied across all Go files in this PR.
ui_texts.go (1)
1-2: LGTM! Header comment follows convention.The header comment addition aligns with the repository-wide convention being applied across all Go files in this PR.
README.md (1)
4-9: LGTM! CI/CD badges enhance project visibility.The addition of GitHub Actions workflow badges and coverage/quality metrics provides valuable information about project health and build status to users and contributors.
theme.go (1)
3-4: Build tags correctly gate Windows-only code.
Looks good; both modern and legacy tags are present and positioned correctly.tray_handlers.go (1)
3-4: Build tags OK.
Gated for Windows correctly.utils.go (3)
3-4: Build tags OK.
Windows-only as intended.
28-34: OK to open HKCU with requested access; error propagation is correct.
No changes needed.
36-38: getExePath is fine and testable.
Returns os.Executable unchanged. Good.main.go (2)
3-4: Build tags OK.
Windows-only main is correctly gated.
23-31: Constructor behavior LGTM.
Defaulting empty version to v0.0.0 matches tests.main_stub.go (1)
3-4: Non-Windows stub gating is correct.
Clear message and exit code 1. All good..codecov.yml (1)
8-44: Codecov config — verified
- coverage.range "70...100" is valid; it defines color cutovers (below 70 = red, 70...100 = yellow, >=100 = green).
- flags.paths accepts shell-style globs (not regex); "." selects the repository root (files under the repo root belong to that flag).
- coverage.ignore accepts glob patterns (e.g. "/*_test.go", "/testdata/**") and will exclude matching files.
.github/dependabot.yml (1)
8-25: LGTM: solid schedules, scopes, review routing.Weekly cadence, reviewers/labels, and rebase-strategy look good.
Consider adding “groups” to reduce PR noise (e.g., toolchain vs. libs). Want a snippet?
Also applies to: 26-44
.github/workflows/test.yml (1)
22-26: Confirm Go version availability across runners.All jobs use Go 1.25. Ensure the repo builds/tests against that toolchain (and update go.mod if needed).
Would you like me to add a matrix to test 1.23–1.25 for smoother upgrades?
Also applies to: 88-91, 153-156
semver_test.go (1)
1-1: LGTM: header comment addition is fine.semver_utils_test.go (1)
10-57: Good coverage for padVersionParts; table-driven style is clean.cross_platform_test.go (1)
1-5: Build tags look correct (new + legacy).pure_funcs_test.go (1)
14-16: If production funcs aren’t available cross-platform, add !windows build tags at file top.Keeps this file compiling on Linux/macOS CI.
Apply:
+//go:build !windows +// +build !windowsTESTING.md (1)
100-103: Coverage gating verified — 70% enforcedTest workflow (.github/workflows/test.yml — test-windows job) contains an explicit "Check coverage threshold" step that fails the job when total coverage < 70%; .codecov.yml also sets project and patch targets to 70% and has codecov.require_ci_to_pass: true.
| // app_test.go | ||
|
|
||
| package main | ||
|
|
||
| import ( | ||
| "testing" | ||
| ) |
There was a problem hiding this comment.
Gate this test to Windows; it depends on Windows-only code.
Without tags, this won’t compile on non-Windows (App/NewApp live in windows-gated files).
Apply:
// app_test.go
+
+//go:build windows
+// +build windows
+
package main📝 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.
| // app_test.go | |
| package main | |
| import ( | |
| "testing" | |
| ) | |
| // app_test.go | |
| //go:build windows | |
| // +build windows | |
| package main | |
| import ( | |
| "testing" | |
| ) |
🤖 Prompt for AI Agents
In app_test.go around lines 1 to 7, the test references Windows-only code and
must be build-gated; add a Windows build tag at the top of the file (use the
modern //go:build windows line and the legacy // +build windows line immediately
before the package declaration) so the test only compiles on Windows, keeping
the rest of the file intact.
| func TestIsVersionNewerPure(t *testing.T) { | ||
| // Extracted logic test without external dependencies | ||
| testVersionNewer := func(latest, current string) bool { | ||
| latest = strings.TrimPrefix(latest, "v") | ||
| current = strings.TrimPrefix(current, "v") | ||
|
|
||
| latestParts := strings.Split(latest, ".") | ||
| currentParts := strings.Split(current, ".") | ||
|
|
||
| // Normalize to same length by padding with zeros | ||
| maxLen := len(latestParts) | ||
| if len(currentParts) > maxLen { | ||
| maxLen = len(currentParts) | ||
| } | ||
|
|
||
| // Pad with zeros | ||
| for len(latestParts) < maxLen { | ||
| latestParts = append(latestParts, "0") | ||
| } | ||
| for len(currentParts) < maxLen { | ||
| currentParts = append(currentParts, "0") | ||
| } | ||
|
|
||
| for i := 0; i < maxLen; i++ { | ||
| latestNum, err1 := strconv.Atoi(latestParts[i]) | ||
| currentNum, err2 := strconv.Atoi(currentParts[i]) | ||
| if err1 != nil || err2 != nil { | ||
| return false | ||
| } | ||
|
|
||
| if latestNum > currentNum { | ||
| return true | ||
| } else if latestNum < currentNum { | ||
| return false | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
These tests reimplement isVersionNewer, so they don’t test production code. Replace with calls to isVersionNewer (and gate with !windows if needed).
Current approach can pass while the real implementation breaks.
Apply this diff:
-func TestIsVersionNewerPure(t *testing.T) {
- // Extracted logic test without external dependencies
- testVersionNewer := func(latest, current string) bool {
- latest = strings.TrimPrefix(latest, "v")
- current = strings.TrimPrefix(current, "v")
- latestParts := strings.Split(latest, ".")
- currentParts := strings.Split(current, ".")
- // Normalize to same length by padding with zeros
- maxLen := len(latestParts)
- if len(currentParts) > maxLen {
- maxLen = len(currentParts)
- }
- // Pad with zeros
- for len(latestParts) < maxLen { latestParts = append(latestParts, "0") }
- for len(currentParts) < maxLen { currentParts = append(currentParts, "0") }
- for i := 0; i < maxLen; i++ {
- latestNum, err1 := strconv.Atoi(latestParts[i])
- currentNum, err2 := strconv.Atoi(currentParts[i])
- if err1 != nil || err2 != nil { return false }
- if latestNum > currentNum { return true } else if latestNum < currentNum { return false }
- }
- return false
- }
+func TestIsVersionNewerPure(t *testing.T) {
@@
- got := testVersionNewer(tt.latest, tt.current)
+ got := isVersionNewer(tt.latest, tt.current)
if got != tt.want {
t.Errorf("testVersionNewer(%q, %q) = %v; want %v", tt.latest, tt.current, got, tt.want)
}If isVersionNewer isn’t available on all platforms, add at file top:
+//go:build !windows
+// +build !windowsAnd drop unused imports accordingly.
📝 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 TestIsVersionNewerPure(t *testing.T) { | |
| // Extracted logic test without external dependencies | |
| testVersionNewer := func(latest, current string) bool { | |
| latest = strings.TrimPrefix(latest, "v") | |
| current = strings.TrimPrefix(current, "v") | |
| latestParts := strings.Split(latest, ".") | |
| currentParts := strings.Split(current, ".") | |
| // Normalize to same length by padding with zeros | |
| maxLen := len(latestParts) | |
| if len(currentParts) > maxLen { | |
| maxLen = len(currentParts) | |
| } | |
| // Pad with zeros | |
| for len(latestParts) < maxLen { | |
| latestParts = append(latestParts, "0") | |
| } | |
| for len(currentParts) < maxLen { | |
| currentParts = append(currentParts, "0") | |
| } | |
| for i := 0; i < maxLen; i++ { | |
| latestNum, err1 := strconv.Atoi(latestParts[i]) | |
| currentNum, err2 := strconv.Atoi(currentParts[i]) | |
| if err1 != nil || err2 != nil { | |
| return false | |
| } | |
| if latestNum > currentNum { | |
| return true | |
| } else if latestNum < currentNum { | |
| return false | |
| } | |
| } | |
| return false | |
| } | |
| func TestIsVersionNewerPure(t *testing.T) { | |
| // Use production implementation directly (don't reimplement isVersionNewer here). | |
| // (preserve existing table of test cases named `tests`) | |
| for _, tt := range tests { | |
| got := isVersionNewer(tt.latest, tt.current) | |
| if got != tt.want { | |
| t.Errorf("isVersionNewer(%q, %q) = %v; want %v", tt.latest, tt.current, got, tt.want) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In pure_funcs_test.go around lines 14 to 53, the test currently reimplements the
isVersionNewer logic instead of exercising the production function; replace the
local testVersionNewer closure with direct calls to the package's isVersionNewer
function so the tests validate real code, and if isVersionNewer is not built on
Windows add a build constraint (//go:build !windows) at the top of this test
file and remove any now-unused imports; ensure all test cases call
isVersionNewer and keep the same assertions.
| func TestPadVersionPartsPure(t *testing.T) { | ||
| padVersionParts := func(parts []string, targetLen int) []string { | ||
| result := make([]string, len(parts)) | ||
| copy(result, parts) | ||
| for len(result) < targetLen { | ||
| result = append(result, "0") | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| parts []string | ||
| targetLen int | ||
| want []string | ||
| }{ | ||
| { | ||
| name: "pad from 2 to 3", | ||
| parts: []string{"1", "2"}, | ||
| targetLen: 3, | ||
| want: []string{"1", "2", "0"}, | ||
| }, | ||
| { | ||
| name: "pad from 1 to 3", | ||
| parts: []string{"1"}, | ||
| targetLen: 3, | ||
| want: []string{"1", "0", "0"}, | ||
| }, | ||
| { | ||
| name: "no padding needed", | ||
| parts: []string{"1", "2", "3"}, | ||
| targetLen: 3, | ||
| want: []string{"1", "2", "3"}, | ||
| }, | ||
| { | ||
| name: "already longer than target", | ||
| parts: []string{"1", "2", "3", "4"}, | ||
| targetLen: 3, | ||
| want: []string{"1", "2", "3", "4"}, | ||
| }, | ||
| { | ||
| name: "empty parts", | ||
| parts: []string{}, | ||
| targetLen: 2, | ||
| want: []string{"0", "0"}, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := padVersionParts(tt.parts, tt.targetLen) | ||
| if !reflect.DeepEqual(got, tt.want) { | ||
| t.Errorf("padVersionParts() = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue: local padVersionParts duplicates production logic. Call the real padVersionParts instead.
Keeps tests meaningful and prevents drift.
Apply this diff:
-func TestPadVersionPartsPure(t *testing.T) {
- padVersionParts := func(parts []string, targetLen int) []string {
- result := make([]string, len(parts))
- copy(result, parts)
- for len(result) < targetLen {
- result = append(result, "0")
- }
- return result
- }
+func TestPadVersionPartsPure(t *testing.T) {
@@
- got := padVersionParts(tt.parts, tt.targetLen)
+ got := padVersionParts(tt.parts, tt.targetLen)Also remove now-unused imports (reflect stays).
📝 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 TestPadVersionPartsPure(t *testing.T) { | |
| padVersionParts := func(parts []string, targetLen int) []string { | |
| result := make([]string, len(parts)) | |
| copy(result, parts) | |
| for len(result) < targetLen { | |
| result = append(result, "0") | |
| } | |
| return result | |
| } | |
| tests := []struct { | |
| name string | |
| parts []string | |
| targetLen int | |
| want []string | |
| }{ | |
| { | |
| name: "pad from 2 to 3", | |
| parts: []string{"1", "2"}, | |
| targetLen: 3, | |
| want: []string{"1", "2", "0"}, | |
| }, | |
| { | |
| name: "pad from 1 to 3", | |
| parts: []string{"1"}, | |
| targetLen: 3, | |
| want: []string{"1", "0", "0"}, | |
| }, | |
| { | |
| name: "no padding needed", | |
| parts: []string{"1", "2", "3"}, | |
| targetLen: 3, | |
| want: []string{"1", "2", "3"}, | |
| }, | |
| { | |
| name: "already longer than target", | |
| parts: []string{"1", "2", "3", "4"}, | |
| targetLen: 3, | |
| want: []string{"1", "2", "3", "4"}, | |
| }, | |
| { | |
| name: "empty parts", | |
| parts: []string{}, | |
| targetLen: 2, | |
| want: []string{"0", "0"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got := padVersionParts(tt.parts, tt.targetLen) | |
| if !reflect.DeepEqual(got, tt.want) { | |
| t.Errorf("padVersionParts() = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } | |
| func TestPadVersionPartsPure(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| parts []string | |
| targetLen int | |
| want []string | |
| }{ | |
| { | |
| name: "pad from 2 to 3", | |
| parts: []string{"1", "2"}, | |
| targetLen: 3, | |
| want: []string{"1", "2", "0"}, | |
| }, | |
| { | |
| name: "pad from 1 to 3", | |
| parts: []string{"1"}, | |
| targetLen: 3, | |
| want: []string{"1", "0", "0"}, | |
| }, | |
| { | |
| name: "no padding needed", | |
| parts: []string{"1", "2", "3"}, | |
| targetLen: 3, | |
| want: []string{"1", "2", "3"}, | |
| }, | |
| { | |
| name: "already longer than target", | |
| parts: []string{"1", "2", "3", "4"}, | |
| targetLen: 3, | |
| want: []string{"1", "2", "3", "4"}, | |
| }, | |
| { | |
| name: "empty parts", | |
| parts: []string{}, | |
| targetLen: 2, | |
| want: []string{"0", "0"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got := padVersionParts(tt.parts, tt.targetLen) | |
| if !reflect.DeepEqual(got, tt.want) { | |
| t.Errorf("padVersionParts() = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In pure_funcs_test.go around lines 85 to 141 the test defines a local
padVersionParts that duplicates production logic; replace the local function
with a direct call to the real padVersionParts from the production package
(remove the inner function declaration), update test calls to use the imported
production function, and ensure the production package is imported under the
correct name/path; finally remove any now-unused imports (keep reflect if still
used).
| func TestMaxPure(t *testing.T) { | ||
| max := func(a, b int) int { | ||
| if a > b { | ||
| return a | ||
| } | ||
| return b | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| a int | ||
| b int | ||
| want int | ||
| }{ | ||
| {"a greater than b", 5, 3, 5}, | ||
| {"b greater than a", 2, 7, 7}, | ||
| {"a equals b", 4, 4, 4}, | ||
| {"negative numbers", -2, -5, -2}, | ||
| {"zero and positive", 0, 1, 1}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := max(tt.a, tt.b) | ||
| if got != tt.want { | ||
| t.Errorf("max(%d, %d) = %d, want %d", tt.a, tt.b, got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue: local max duplicates production logic. Use max(a,b) from code.
Prevents false confidence.
Apply this diff:
-func TestMaxPure(t *testing.T) {
- max := func(a, b int) int {
- if a > b {
- return a
- }
- return b
- }
+func TestMaxPure(t *testing.T) {
@@
- got := max(tt.a, tt.b)
+ got := max(tt.a, tt.b)📝 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 TestMaxPure(t *testing.T) { | |
| max := func(a, b int) int { | |
| if a > b { | |
| return a | |
| } | |
| return b | |
| } | |
| tests := []struct { | |
| name string | |
| a int | |
| b int | |
| want int | |
| }{ | |
| {"a greater than b", 5, 3, 5}, | |
| {"b greater than a", 2, 7, 7}, | |
| {"a equals b", 4, 4, 4}, | |
| {"negative numbers", -2, -5, -2}, | |
| {"zero and positive", 0, 1, 1}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got := max(tt.a, tt.b) | |
| if got != tt.want { | |
| t.Errorf("max(%d, %d) = %d, want %d", tt.a, tt.b, got, tt.want) | |
| } | |
| }) | |
| } | |
| } | |
| func TestMaxPure(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| a int | |
| b int | |
| want int | |
| }{ | |
| {"a greater than b", 5, 3, 5}, | |
| {"b greater than a", 2, 7, 7}, | |
| {"a equals b", 4, 4, 4}, | |
| {"negative numbers", -2, -5, -2}, | |
| {"zero and positive", 0, 1, 1}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got := max(tt.a, tt.b) | |
| if got != tt.want { | |
| t.Errorf("max(%d, %d) = %d, want %d", tt.a, tt.b, got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In pure_funcs_test.go around lines 143-172, the test redefines a local max
function that duplicates production logic; remove that local max definition and
call the actual max implementation from the package under test instead (delete
the local func, reference the package's max/Max function directly—adjust the
test package or import alias if needed so the production symbol is accessible),
keep the same table-driven cases and assertions, and run go test to confirm no
behavioral duplication remains.
| func TestUITextsValidation(t *testing.T) { | ||
| // Test menu titles structure without importing Windows dependencies | ||
| expectedTexts := map[string]string{ | ||
| "AppName": "🔗 WinGoDarkTray", | ||
| "EnableAutorun": "Enable Autorun", | ||
| "Quit": "✕ Quit", | ||
| "EnableAutorunChecked": "✔ Autorun Enabled", | ||
| "EnableAutorunUnchecked": "✗ Autorun Disabled", | ||
| "ToggleAppToDark": "☾ Toggle app theme to Dark", | ||
| "ToggleAppToLight": "☼ Toggle app theme to Light", | ||
| "ToggleWinToDark": "☾ Toggle Windows theme to Dark", | ||
| "ToggleWinToLight": "☼ Toggle Windows theme to Light", | ||
| "ToggleBothToDark": "☾ Toggle both to Dark", | ||
| "ToggleBothToLight": "☼ Toggle both to Light", | ||
| "UpdateNow": "🔄 Update Now", | ||
| } | ||
|
|
||
| for name, expected := range expectedTexts { | ||
| if expected == "" { | ||
| t.Errorf("Expected text for %s should not be empty", name) | ||
| } | ||
| if name == "AppName" && !strings.Contains(expected, "WinGoDarkTray") { | ||
| t.Errorf("AppName should contain 'WinGoDarkTray', got: %s", expected) | ||
| } | ||
| } | ||
|
|
||
| // Test tooltip structure | ||
| defaultTooltip := "A windows app to toggle light and dark mode from the system tray" | ||
| errorTooltip := "An error occurred, please try again later." | ||
|
|
||
| if !strings.Contains(defaultTooltip, "toggle") || !strings.Contains(defaultTooltip, "windows") { | ||
| t.Error("Default tooltip should describe app purpose") | ||
| } | ||
| if errorTooltip == "" { | ||
| t.Error("Error tooltip should not be empty") | ||
| } | ||
| } |
There was a problem hiding this comment.
UI text “tests” only validate constants defined inside the test; they never exercise production values. Remove or import real constants.
Either import from the code (move texts to a cross-platform file) or drop this block.
Apply this diff to remove the non-valuable test:
-func TestUITextsValidation(t *testing.T) {
- // Test menu titles structure without importing Windows dependencies
- expectedTexts := map[string]string{
- "AppName": "🔗 WinGoDarkTray",
- "EnableAutorun": "Enable Autorun",
- "Quit": "✕ Quit",
- "EnableAutorunChecked": "✔ Autorun Enabled",
- "EnableAutorunUnchecked": "✗ Autorun Disabled",
- "ToggleAppToDark": "☾ Toggle app theme to Dark",
- "ToggleAppToLight": "☼ Toggle app theme to Light",
- "ToggleWinToDark": "☾ Toggle Windows theme to Dark",
- "ToggleWinToLight": "☼ Toggle Windows theme to Light",
- "ToggleBothToDark": "☾ Toggle both to Dark",
- "ToggleBothToLight": "☼ Toggle both to Light",
- - "UpdateNow": "🔄 Update Now",
- }
- for name, expected := range expectedTexts {
- if expected == "" {
- t.Errorf("Expected text for %s should not be empty", name)
- }
- if name == "AppName" && !strings.contains(expected, "WinGoDarkTray") {
- t.Errorf("AppName should contain 'WinGoDarkTray', got: %s", expected)
- }
- }
- // Tooltips...
-}If you want to keep UI text tests, move the text constants to a platform-agnostic file and import them here.
📝 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 TestUITextsValidation(t *testing.T) { | |
| // Test menu titles structure without importing Windows dependencies | |
| expectedTexts := map[string]string{ | |
| "AppName": "🔗 WinGoDarkTray", | |
| "EnableAutorun": "Enable Autorun", | |
| "Quit": "✕ Quit", | |
| "EnableAutorunChecked": "✔ Autorun Enabled", | |
| "EnableAutorunUnchecked": "✗ Autorun Disabled", | |
| "ToggleAppToDark": "☾ Toggle app theme to Dark", | |
| "ToggleAppToLight": "☼ Toggle app theme to Light", | |
| "ToggleWinToDark": "☾ Toggle Windows theme to Dark", | |
| "ToggleWinToLight": "☼ Toggle Windows theme to Light", | |
| "ToggleBothToDark": "☾ Toggle both to Dark", | |
| "ToggleBothToLight": "☼ Toggle both to Light", | |
| "UpdateNow": "🔄 Update Now", | |
| } | |
| for name, expected := range expectedTexts { | |
| if expected == "" { | |
| t.Errorf("Expected text for %s should not be empty", name) | |
| } | |
| if name == "AppName" && !strings.Contains(expected, "WinGoDarkTray") { | |
| t.Errorf("AppName should contain 'WinGoDarkTray', got: %s", expected) | |
| } | |
| } | |
| // Test tooltip structure | |
| defaultTooltip := "A windows app to toggle light and dark mode from the system tray" | |
| errorTooltip := "An error occurred, please try again later." | |
| if !strings.Contains(defaultTooltip, "toggle") || !strings.Contains(defaultTooltip, "windows") { | |
| t.Error("Default tooltip should describe app purpose") | |
| } | |
| if errorTooltip == "" { | |
| t.Error("Error tooltip should not be empty") | |
| } | |
| } |
| func TestOpenBrowserOnNonWindows(t *testing.T) { | ||
| if runtime.GOOS == "windows" { | ||
| t.Skip("Skipping non-Windows test on Windows platform") | ||
| } | ||
|
|
||
| // This test verifies that openBrowser() handles non-Windows platforms gracefully | ||
| // Since showError() would be called, we can't easily test this without mocking | ||
| // But we can at least verify the function doesn't panic | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| t.Errorf("openBrowser() panicked on non-Windows platform: %v", r) | ||
| } | ||
| }() | ||
|
|
||
| // Note: This will call showError() internally but shouldn't panic | ||
| openBrowser("https://example.com") | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Non-Windows openBrowser test intention is good; ensure the symbol exists.
With the stub above, this will compile and assert no panic without side effects.
After adding the stub, run:
🏁 Script executed:
#!/bin/bash
# Validate tests compile on Linux and Windows (CI matrices)
go test ./... -cLength of output: 597
Add stubs/implementations for missing symbols so tests compile.
go test -c failed with undefined symbols — add minimal test-safe stubs or implement the referenced functions/vars:
- app_test.go:34 — undefined: NewApp
- updater_logic_test.go:48,49,53,57,61 — undefined: githubReleasesAPI
- utils_test.go:12 — undefined: getExePath
- utils_test.go:42 — undefined: openBrowser
Place stubs in a *_test.go helper file in the same package (ensure openBrowser stub avoids side effects). After fixes re-run: go test ./... -c
🤖 Prompt for AI Agents
In utils_test.go around lines 27 to 43 and related tests, the package is missing
test-only symbols causing go test -c to fail (NewApp, githubReleasesAPI,
getExePath, openBrowser); add a new helper file named something like
test_helpers_stub_test.go in the same package that defines minimal, test-safe
stubs: implement NewApp returning a simple mock app, define githubReleasesAPI as
a lightweight interface/variable used by updater tests, implement getExePath
returning a deterministic path and no error, and implement openBrowser as a
no-op (or record the URL in a package-level variable) to avoid side effects;
keep implementations minimal and safe for concurrent tests, put them in _test.go
so they don’t ship in production, then re-run go test ./... -c.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
.gitignore (1)
1-1: Don't blanket-ignore all dotfiles; add explicit exceptions for repo configs.
.*will hide useful config like .editorconfig and .gitattributes unless explicitly whitelisted.Apply:
.* +!.gitattributes +!.editorconfig +!.gitmodules.golangci.yml (2)
6-10: Broaden lint coverage (staticcheck/revive/unused/ineffassign/misspell/errcheck).These catch real issues missed by gofmt/goimports/govet alone.
Apply:
linters: enable: - gofmt - goimports - govet + - staticcheck + - revive + - unused + - ineffassign + - misspell + - errcheck + - gosimple
3-4: Optional: set build tags if linting on non‑Windows runners.Prevents false compile failures when Windows code is gated by tags.
Example:
run: timeout: 5m + build-tags: + - windowsupdater_stub.go (1)
3-4: Legacy build tag is redundant on modern Go.Keep only
//go:buildif supporting Go ≥1.18; otherwise fine to keep both.-// +build !windowsutils_stub.go (1)
8-18: Use a sentinel error for non‑Windows stubs (enable errors.Is checks).Returning new fmt.Errorf each call hinders robust comparisons.
Apply:
-package main - -import "fmt" +package main + +import "errors" @@ -func getExePath() (string, error) { - return "", fmt.Errorf("getExePath not supported on non-Windows platforms") -} +var ErrUnsupportedPlatform = errors.New("not supported on non-Windows platforms") + +func getExePath() (string, error) { + return "", ErrUnsupportedPlatform +} @@ -func openBrowser(url string) error { - return fmt.Errorf("openBrowser not supported on non-Windows platforms") -} +func openBrowser(url string) error { + return ErrUnsupportedPlatform +}Ensure tests use errors.Is(..., ErrUnsupportedPlatform) rather than string matching.
app_stub.go (2)
29-32: Return a shared sentinel error (align with utils_stub).Provides consistent detection across stubs.
Apply:
-import "fmt" +import "errors" @@ -func (a *App) Run() error { - return fmt.Errorf("App.Run not supported on non-Windows platforms") -} +var ErrUnsupportedPlatform = errors.New("not supported on non-Windows platforms") + +func (a *App) Run() error { + return ErrUnsupportedPlatform +}If both utils_stub.go and app_stub.go define ErrUnsupportedPlatform, consolidate into one (single source) or alias to avoid duplicates.
10-17: Interfaces as field types are fine for a stub; consider omitting fields entirely.If tests don’t reference these, trimming them avoids leaking stub‑only surface.
.github/workflows/security.yml (3)
34-38: Pin tool installs; avoid floating @latest.Floating installs reduce reproducibility.
Example:
- run: go install golang.org/x/vuln/cmd/govulncheck@latest + run: go install golang.org/x/vuln/cmd/govulncheck@v1.1.0 @@ - run: go install honnef.co/go/tools/cmd/staticcheck@latest + run: go install honnef.co/go/tools/cmd/staticcheck@2024.1.1Please confirm the latest stable versions before pinning.
Also applies to: 102-106
82-87: This step doesn’t audit vulnerabilities.
go list -json -depsonly lists deps; it doesn’t check advisories. Either remove the job (govulncheck already covers it) or repurpose to integrity checks.Apply:
- - name: Run go mod audit (built-in vulnerability check) - run: | - echo "Checking for known vulnerabilities in dependencies..." - go list -json -deps ./... | findstr "Module" || echo "Dependency scan completed" - echo "Note: Using govulncheck for comprehensive vulnerability analysis" - continue-on-error: true + - name: Verify modules + run: | + go mod verify + go mod tidy -diff
21-39: Optional: run non‑Windows‑specific jobs on ubuntu‑latest to speed up.govulncheck/staticcheck/go vet don’t require Windows and run faster/cheaper on Linux.
- runs-on: windows-latest + runs-on: ubuntu-latestAlso applies to: 89-121
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/build-and-publish.yml(4 hunks).github/workflows/security.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks).golangci.yml(1 hunks)app_stub.go(1 hunks)updater_stub.go(1 hunks)utils_stub.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/build-and-publish.yml
- .github/workflows/test.yml
⏰ 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). (4)
- GitHub Check: Static Security Analysis
- GitHub Check: Dependency Vulnerability Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: Test on Windows
🔇 Additional comments (3)
.github/workflows/security.yml (1)
41-65: CodeQL job looks good.Pinned to v3 actions with proper permissions and categories.
.gitignore (1)
6-9: Pre‑commit filename mismatch; ensure both variants are unignored.File: .gitignore
Lines: 6-9Snippet:
!.codecov.yml !.github/ !.golangci.yml !.pre-commit-config.ymlApply:
!.codecov.yml !.github/ !.golangci.yml !.pre-commit-config.yml +!.pre-commit-config.yamlVerification: prior check produced no output — confirm locally whether .pre-commit-config.yaml exists/tracked by running:
git ls-files --cached --others --exclude-standard | grep -E '^.pre-commit-config.(ya?ml)$' -n || trueupdater_stub.go (1)
8-9: No action required — tests can access the unexported constant.
updater_stub.go (//go:build !windows) and updater.go define githubReleasesAPI; updater_logic_test.go is in package main (not main_test) and references it, so exporting the constant is unnecessary.Likely an incorrect or invalid review comment.
| - name: Set up Go | ||
| uses: actions/setup-go@v5 | ||
| with: | ||
| go-version: '1.25' | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Don’t hardcode Go 1.25; align with go.mod.
Use setup-go’s go-version-file to follow the repo’s declared version; avoids breakage if 1.25 isn’t available.
Apply:
- - name: Set up Go
- uses: actions/setup-go@v5
- with:
- go-version: '1.25'
+ - name: Set up Go
+ uses: actions/setup-go@v5
+ with:
+ go-version-file: 'go.mod'Also applies to: 74-78, 97-101
🏁 Script executed:
#!/bin/bash
# Show declared Go toolchain in go.mod
rg -n '^go\s+\d+\.\d+' go.mod -n || cat go.modLength of output: 11
🌐 Web query:
Is Go 1.25 released and supported by actions/setup-go@v5? What is the latest stable Go version?
💡 Result:
Yes — Go 1.25 is released (go1.25.0 released August 12, 2025) and the official Go site shows the 1.25 release. [1]
actions/setup-go@v5 can install any Go release listed in the actions/go-versions manifest (and will download versions JIT if not in the runner image), so you can use setup-go@v5 with go-version: '1.25' (or 'stable') to get Go 1.25. [2][3]
Sources:
- Go 1.25 release announcement and release notes. [1]
- actions/setup-go README (v5) showing go-version usage and that it pulls versions from actions/go-versions. [2]
- actions/go-versions repository (versions-manifest used by setup-go). [3]
Align workflow Go version to go.mod (don't hardcode 1.25).
go.mod declares "go 1.23.1" while the workflow hardcodes 1.25 — replace the hardcoded go-version with setup-go's go-version-file to follow go.mod and avoid CI/toolchain drift.
Applies to: .github/workflows/security.yml — lines 29-33, 74-78, 97-101.
- - name: Set up Go
- uses: actions/setup-go@v5
- with:
- go-version: '1.25'
+ - name: Set up Go
+ uses: actions/setup-go@v5
+ with:
+ go-version-file: 'go.mod'📝 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.
| - name: Set up Go | |
| uses: actions/setup-go@v5 | |
| with: | |
| go-version: '1.25' | |
| - name: Set up Go | |
| uses: actions/setup-go@v5 | |
| with: | |
| go-version-file: 'go.mod' |
🤖 Prompt for AI Agents
In .github/workflows/security.yml around lines 29-33, 74-78 and 97-101, the
workflow currently hardcodes go-version: '1.25'; replace that with the setup-go
input that reads the Go version from the repository (use setup-go's
go-version-file pointing to go.mod) so the CI uses the go version declared in
go.mod; update each occurrence to use go-version-file: 'go.mod' and remove the
hardcoded go-version value.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
icon_test.go (1)
61-70: Tighten failure semantics in TestIconEmbedded.If icon is nil/empty, fail fast to avoid continuing a meaningless test run. Keep header mismatch as warning if that leniency is intentional.
@@ - if icon == nil { - t.Error("icon byte slice is nil") - } - if len(icon) == 0 { - t.Error("icon byte slice is empty") - } + if icon == nil { + t.Fatal("icon byte slice is nil") + } + if len(icon) == 0 { + t.Fatal("icon byte slice is empty") + }Also applies to: 71-80
tray_handlers_test.go (1)
29-62: Guard against implicit systray requirements in NewApp or menu item creation.If NewApp touches systray API, this can hang/panic on CI. Prefer DI (menu factory) so tests use stubs; otherwise skip under -short/CI by default.
Minimal guard if you can’t refactor now:
func TestAppMenuItemInitialization(t *testing.T) { + if testing.Short() { + t.Skip("skipping systray-adjacent test in -short") + } app := NewApp("test-version")Longer-term: define an interface for menu creation (e.g., type MenuFactory interface { NewItem(title string) *systray.MenuItem }) and inject a fake in tests.
autorun_test.go (2)
14-38: UI test intentionally skipped — consider unit-testing logic via seam.If updateAutorunUI has pure logic (title selection), expose that logic behind a small function and unit-test it without systray.
40-93: Good isolated HKCU test key usage; switch to t.Cleanup for robust teardown.This ensures cleanup runs even if the test fails early.
@@ - defer func() { - testKey.Close() - // Clean up the test key - registry.DeleteKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayTest") - }() + t.Cleanup(func() { + testKey.Close() + _ = registry.DeleteKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayTest") + })logging_test.go (3)
53-103: Constant checks OK; optional: assert types to catch accidental type drift.Not required, but you could assert reflect.TypeOf(...) if regressions have occurred before.
105-165: Console and safe logging tests are “no panic” smoke tests — consider capturing output to strengthen assertions.You can capture stdout/stderr with os.Pipe and verify prefixes [INFO]/[WARNING]/[ERROR] if that’s the expected format.
220-247: Verify showNotificationWithFallback can run headless.If it touches systray, consider skipping under -short or injecting a notifier that falls back to logging in tests.
theme_test.go (1)
96-111: Reading real theme from HKCU: treat as integration and skip by default under -short.Avoid CI flakiness on machines without that key.
func TestGetCurrentAppThemeMode(t *testing.T) { + if testing.Short() { + t.Skip("integration: reads HKCU theme registry") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)autorun_test.go(1 hunks)icon_test.go(1 hunks)logging_test.go(1 hunks)theme_test.go(1 hunks)tray_handlers_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
⏰ 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). (4)
- GitHub Check: Test on Windows
- GitHub Check: Dependency Vulnerability Scan
- GitHub Check: Static Security Analysis
- GitHub Check: CodeQL Analysis
🔇 Additional comments (11)
tray_handlers_test.go (2)
12-27: Skip is appropriate for systray-dependent handler.Acknowledged and appropriate to avoid flaky UI/threading in CI.
64-95: Version field checks look good.autorun_test.go (1)
95-132: withAutorunRegistry read-access cases look fine.Consider adding one negative case only if it won’t mutate real autorun state; otherwise keep it as-is to avoid side effects.
logging_test.go (3)
15-51: Event type name mapping assertions are solid.
167-192: Confirm showError has no blocking UI in CI.If it can open dialogs, guard under -short/CI or route through a no-op notifier in tests.
194-218: Appropriate skip for tooltip test.theme_test.go (4)
14-55: Theme constants test looks good.
57-94: Registry access wrapper tests are fine.
158-197: UI/registry-dependent methods correctly skipped.
199-220: Single-theme toggles correctly skipped.icon_test.go (1)
9-50: Fix: make Windows-only constant checks Windows-onlyRemove the autorunRegistryKey case from icon_test.go (ungated) and add a Windows-only test file icon_windows_test.go with the //go:build windows tag that asserts autorunRegistryKey ==
Software\Microsoft\Windows\CurrentVersion\Run. Repository contains Windows-gated files (e.g., utils.go, autorun.go); confirm where autorunRegistryKey is defined and gated before applying the change.
| func TestSetBothThemeModes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| lightMode bool | ||
| }{ | ||
| { | ||
| name: "set to light mode", | ||
| lightMode: true, | ||
| }, | ||
| { | ||
| name: "set to dark mode", | ||
| lightMode: false, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // Note: This test may fail if we don't have write access to the registry | ||
| // or if we're not running with sufficient privileges | ||
| err := setBothThemeModes(tt.lightMode) | ||
| if err != nil { | ||
| t.Logf("Expected behavior on restricted systems: %v", err) | ||
| // Don't fail the test as this is expected behavior in many environments | ||
| return | ||
| } | ||
|
|
||
| // If successful, verify the values were set correctly | ||
| appMode, err := getCurrentAppThemeMode() | ||
| if err != nil { | ||
| t.Logf("Could not verify app mode after setting: %v", err) | ||
| return | ||
| } | ||
|
|
||
| expectedMode := uint64(0) | ||
| if tt.lightMode { | ||
| expectedMode = 1 | ||
| } | ||
|
|
||
| if appMode != expectedMode { | ||
| t.Errorf("After setBothThemeModes(%v), app mode = %d, want %d", tt.lightMode, appMode, expectedMode) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: this test can change the user’s Windows theme (HKCU). Do not mutate user settings in unit tests.
setBothThemeModes writes AppsUseLightTheme/SystemUsesLightTheme; on a real Windows runner it will flip the user’s theme. Skip by default unless explicitly opted-in via env, or refactor to inject a registry interface and test against a fake.
Minimal safety guard:
@@
-import (
- "testing"
-
- "golang.org/x/sys/windows/registry"
-)
+import (
+ "os"
+ "testing"
+
+ "golang.org/x/sys/windows/registry"
+)
@@
func TestSetBothThemeModes(t *testing.T) {
+ if testing.Short() || os.Getenv("ALLOW_THEME_REG_WRITE") != "1" {
+ t.Skip("integration: would modify HKCU theme; set ALLOW_THEME_REG_WRITE=1 to run explicitly")
+ }Preferred: define a Registry interface (OpenKey/GetInteger/SetInteger) and pass a fake to setBothThemeModes in tests.
📝 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 TestSetBothThemeModes(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| lightMode bool | |
| }{ | |
| { | |
| name: "set to light mode", | |
| lightMode: true, | |
| }, | |
| { | |
| name: "set to dark mode", | |
| lightMode: false, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| // Note: This test may fail if we don't have write access to the registry | |
| // or if we're not running with sufficient privileges | |
| err := setBothThemeModes(tt.lightMode) | |
| if err != nil { | |
| t.Logf("Expected behavior on restricted systems: %v", err) | |
| // Don't fail the test as this is expected behavior in many environments | |
| return | |
| } | |
| // If successful, verify the values were set correctly | |
| appMode, err := getCurrentAppThemeMode() | |
| if err != nil { | |
| t.Logf("Could not verify app mode after setting: %v", err) | |
| return | |
| } | |
| expectedMode := uint64(0) | |
| if tt.lightMode { | |
| expectedMode = 1 | |
| } | |
| if appMode != expectedMode { | |
| t.Errorf("After setBothThemeModes(%v), app mode = %d, want %d", tt.lightMode, appMode, expectedMode) | |
| } | |
| }) | |
| } | |
| } | |
| import ( | |
| "os" | |
| "testing" | |
| "golang.org/x/sys/windows/registry" | |
| ) | |
| func TestSetBothThemeModes(t *testing.T) { | |
| if testing.Short() || os.Getenv("ALLOW_THEME_REG_WRITE") != "1" { | |
| t.Skip("integration: would modify HKCU theme; set ALLOW_THEME_REG_WRITE=1 to run explicitly") | |
| } | |
| tests := []struct { | |
| name string | |
| lightMode bool | |
| }{ | |
| { | |
| name: "set to light mode", | |
| lightMode: true, | |
| }, | |
| { | |
| name: "set to dark mode", | |
| lightMode: false, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| // Note: This test may fail if we don't have write access to the registry | |
| // or if we're not running with sufficient privileges | |
| err := setBothThemeModes(tt.lightMode) | |
| if err != nil { | |
| t.Logf("Expected behavior on restricted systems: %v", err) | |
| // Don't fail the test as this is expected behavior in many environments | |
| return | |
| } | |
| // If successful, verify the values were set correctly | |
| appMode, err := getCurrentAppThemeMode() | |
| if err != nil { | |
| t.Logf("Could not verify app mode after setting: %v", err) | |
| return | |
| } | |
| expectedMode := uint64(0) | |
| if tt.lightMode { | |
| expectedMode = 1 | |
| } | |
| if appMode != expectedMode { | |
| t.Errorf("After setBothThemeModes(%v), app mode = %d, want %d", tt.lightMode, appMode, expectedMode) | |
| } | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In theme_test.go lines 113-156, the test mutates HKCU theme keys which can
change a user’s Windows theme; change the test to avoid touching real registry
by either (A) gating the test behind an explicit opt-in env var (e.g.,
SKIP_THEME_TESTS=false to run; otherwise call t.Skip) or (preferred) refactor
setBothThemeModes to accept a Registry interface (OpenKey/GetInteger/SetInteger)
and update production call sites to pass the real registry implementation; then
in this test inject a fake Registry that records reads/writes so you can assert
expected SetInteger calls and verify appMode without modifying the real
registry.
There was a problem hiding this comment.
Actionable comments posted: 28
♻️ Duplicate comments (1)
utils_test.go (1)
5-14: Compilation breaks on non-Windows due to unconditional windows/registry import and Windows-only symbols.This file imports golang.org/x/sys/windows/registry and calls openRegistryKey, which aren’t available on non-Windows; typecheck fails.
Apply this diff to gate the file to Windows:
-// utils_test.go +// utils_test.go +//go:build windows +// +build windowsThen move the non-Windows test(s) (e.g., TestOpenBrowserOnNonWindows) into a separate file compiled only on non-Windows (see snippet below).
🧹 Nitpick comments (26)
utils_coverage_test.go (2)
52-79: Non-Windows behavior test is inside a Windows-only file.This test will never run on non-Windows due to the file’s //go:build windows tag.
Move it to a new file compiled only on non-Windows (utils_nonwindows_test.go) and drop the redundant GOOS checks here.
34-36: Redundant GOOS guard inside a Windows-only test file.Given //go:build windows, this check is unnecessary.
Remove the guard to simplify.
advanced_coverage_test.go (1)
284-293: Use registry constants instead of magic numbers; fix duplicate access mask.Both cases use 0x20019; prefer explicit constants and vary access as intended.
Apply this diff:
- access: 0x20019, + access: registry.QUERY_VALUE, @@ - access: 0x20019, + access: registry.READ,utils_test.go (1)
32-91: Relocate TestOpenBrowserOnNonWindows to a non-Windows test file.Keep Windows-only tests here; run non-Windows behavior elsewhere.
Add this new file:
// utils_nonwindows_test.go //go:build !windows // +build !windows package main import ( "os" "path/filepath" "runtime" "testing" ) func TestGetExePath_NonWindows(t *testing.T) { path, err := getExePath() if err != nil { t.Fatalf("getExePath() error: %v", err) } if path == "" { t.Fatal("getExePath() returned empty path") } if !filepath.IsAbs(path) { t.Errorf("getExePath() returned relative path: %s", path) } if _, err := os.Stat(path); os.IsNotExist(err) { t.Errorf("getExePath() returned non-existent path: %s", path) } } func TestOpenBrowserOnNonWindows(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("non-Windows-only test") } defer func() { if r := recover(); r != nil { t.Errorf("openBrowser() panicked: %v", r) } }() openBrowser("https://example.com") // should be a no-op/stub without side effects }main_test.go (2)
12-27: Skip-only tests provide negligible value; consolidate or gate them.These subtests only call t.Skip and inflate runtime/noise. Either:
- Remove them, or
- Collapse into a single placeholder test, or
- Gate the whole file behind an opt-in tag/env to avoid running in CI by default.
Apply this minimal consolidation:
-func TestMainFunctionStructure(t *testing.T) { - ... - t.Skip("main() function starts full application - skipping to avoid side effects") -} -... -func TestOnExitFunction(t *testing.T) { ... t.Skip("onExit function is not exported - cannot test directly") } -... -func TestApplicationInitializationFunctions(t *testing.T) { ... t.Skip("application initialization functions are not exported - cannot test directly") } -... -func TestCreateMenuItems(t *testing.T) { ... t.Skip("createMenuItems requires systray initialization") } -... -func TestStartEventHandlers(t *testing.T) { ... t.Skip("startEventHandlers creates goroutines and requires systray - skipping to avoid side effects") } +func TestWindowsMain_SkippedPlaceholders(t *testing.T) { + t.Skip("Placeholder coverage for unexported/systray-dependent funcs (intentionally skipped).") +}Also applies to: 29-44, 46-70, 72-87, 89-105
106-143: Version test duplicates prod logic; extract a helper and test that.Testing a locally reimplemented branch is brittle. Expose a small helper (e.g., resolveVersion(build string) string) and assert against it; this gives real signal and prevents drift.
Example prod helper (outside this file):
// version.go package main func resolveVersion(build string) string { if build == "" { return "v0.0.0" } return build }Then in this test:
- version := tt.buildVersion - if version == "" { - version = "v0.0.0" - } - if version != tt.expectedVersion { ... } + if got := resolveVersion(tt.buildVersion); got != tt.expectedVersion { + t.Errorf("resolveVersion(%q) = %q, want %q", tt.buildVersion, got, tt.expectedVersion) + }logging_coverage_test.go (3)
69-89: Brittle error-string equality; prefer Contains or sentinel.Matching "unknown event type: 999" verbatim will flake if wording changes. Use substring or errors.Is with a sentinel.
-import ( - "testing" - "time" - - "golang.org/x/sys/windows/svc/eventlog" -) +import ( + "strings" + "testing" + "time" + "golang.org/x/sys/windows/svc/eventlog" +) ... - if tt.eventType == 999 && err != nil { - expectedError := "unknown event type: 999" - if err.Error() != expectedError { - t.Errorf("logEvent() error = %v, want %v", err, expectedError) - } - } + if tt.eventType == 999 && err != nil && !strings.Contains(err.Error(), "unknown event type") { + t.Errorf("logEvent() unexpected error: %v", err) + }
100-112: Gate admin-requiring paths to avoid CI breakage.installEventLogSource usually needs elevation. Skip unless explicitly enabled.
+import "os" ... t.Run(tt.name, func(t *testing.T) { + if os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping admin-requiring test; set ALLOW_WINDOWS_E2E=1 to run.") + } err := installEventLogSource() _ = err })
176-189: Avoid showing real notifications by default.This triggers OS UX in test runs. Gate it behind an env var.
+import "os" ... t.Run(tt.name, func(t *testing.T) { + if os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping notification test; set ALLOW_WINDOWS_E2E=1 to run.") + } defer func() { if r := recover(); r != nil { t.Errorf("... %v", r) } }() showNotificationWithFallback(tt.title, tt.message, tt.fallbackTooltip) })autorun_test.go (3)
66-81: Use uniquely-named temp subkeys to prevent cross-run collisions.A fixed key path can collide across parallel runs or locally. Append PID to the subkey.
- testKey, err := registry.OpenKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayTest", registry.ALL_ACCESS) + keyPath := fmt.Sprintf("Software\\WinGoDarkTrayTest_%d", os.Getpid()) + testKey, err := registry.OpenKey(registry.CURRENT_USER, keyPath, registry.ALL_ACCESS) ... - testKey, _, err = registry.CreateKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayTest", registry.ALL_ACCESS) + testKey, _, err = registry.CreateKey(registry.CURRENT_USER, keyPath, registry.ALL_ACCESS) ... - registry.DeleteKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayTest") + registry.DeleteKey(registry.CURRENT_USER, keyPath)Add import:
-import ( +import ( "fmt" + "os" "runtime" "testing" "golang.org/x/sys/windows/registry" )
170-178: Mirror unique key strategy in edge-case tests.Apply the same dynamic subkey to avoid collisions.
- testKey, _, err := registry.CreateKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayAutorunTest", registry.ALL_ACCESS) + keyPath := fmt.Sprintf("Software\\WinGoDarkTrayAutorunTest_%d", os.Getpid()) + testKey, _, err := registry.CreateKey(registry.CURRENT_USER, keyPath, registry.ALL_ACCESS) ... - registry.DeleteKey(registry.CURRENT_USER, "Software\\WinGoDarkTrayAutorunTest") + registry.DeleteKey(registry.CURRENT_USER, keyPath)
97-134: withAutorunRegistry tests: clarify expectations with an assertion.These paths currently only mark “called”. Consider asserting that the callback executed under the expected access (e.g., read succeeds). If environment variance makes this flaky, leave as-is.
If you want, I can add a follow-up test that verifies a read-only query actually attempts to read a value and tolerates “value not found” specifically.
windows_native_test.go (3)
188-198: Prefer printable long strings over NUL padding.string(make([]byte, 200)) creates NULs and can lead to odd behavior. Use Repeat.
- showNotificationWithFallback("Long Title", string(make([]byte, 200)), "Long tooltip") + showNotificationWithFallback("Long Title", strings.Repeat("x", 200), "Long tooltip")
206-276: Error-path tests also touch OS; gate them.Registry access and getExePath/opening cwd can vary. Make these opt-in for CI stability.
func TestWindowsErrorPaths(t *testing.T) { + if os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping Windows error-path E2E; set ALLOW_WINDOWS_E2E=1 to run.") + }
278-328: Comprehensive API logging test: gate to avoid noisy CI.Logging to event log/console in loops is noisy and environment-sensitive.
func TestWindowsAPISafeInteractions(t *testing.T) { + if os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping Windows API logging E2E; set ALLOW_WINDOWS_E2E=1 to run.") + }theme_coverage_test.go (2)
156-165: Unnecessary NewApp construction; remove side-effect risk.This test only validates constants. Constructing
NewAppadds systray coupling with no assertions.Apply:
- // Test that the constants can be used in theme operations - app := NewApp("test-version") - defer func() { - if r := recover(); r != nil { - t.Errorf("Using registry constant %s caused panic: %v", tt.name, r) - } - }() - - // Skip actual function call as it requires systray initialization - _ = app + // No-op: constants validated above; avoid systray/app side effects
21-27: Placeholder tests that always Skip add noise.If these are scaffolding for future systray E2E, mark them with a dedicated build tag (e.g.,
windows && e2e) or a TODO referencing an issue, so they don’t inflate test counts.Also applies to: 48-54, 65-71, 82-88, 99-105
targeted_coverage_test.go (1)
98-105: Nil or zero-value systray.MenuItem usage risks blocking panics.These code paths couple tests to systray internals. Prefer a minimal interface (Check/Uncheck/SetTitle/Show/Hide) and inject a fake in production code, or skip under
-short.Also applies to: 108-121, 123-136
comprehensive_coverage_test.go (2)
14-15: Import strings for resilient error assertions.Apply:
import ( "fmt" "net/http" "net/http/httptest" "testing" - "golang.org/x/sys/windows/svc/eventlog" + "golang.org/x/sys/windows/svc/eventlog" + "strings" )
39-55: Brittle exact-match error assertion.
logEventerror text may change; assert usingstrings.Contains(err.Error(), "unknown event type")instead of full equality.- if err.Error() != expectedMsg { - t.Errorf("Expected error message %q, got %q", expectedMsg, err.Error()) - } + if !strings.Contains(err.Error(), "unknown event type") { + t.Errorf("Expected error containing %q, got %q", "unknown event type", err.Error()) + }updater_coverage_test.go (2)
228-254: Punctuation nit in expected message.
"available!."reads odd; consider standardizing to"available!".
19-55: Sleep-based timing is brittle.Use a context/WaitGroup or a start signal channel from the handler to make the test deterministic; avoid
time.Sleep.final_coverage_test.go (1)
285-311: Timeout test doesn’t exercise slow server.Since production uses a fixed URL, the
slowServerisn’t hit. After adding an injectable base URL, rewire this test; otherwise remove.updater_test.go (3)
19-24: Hard-coded GitHub API path reduces flexibility.If the repo is forked or moved, this test fails. Consider deriving from a single prod constant/var and overriding in tests when needed.
43-114: Entire test is skipped; prefer injectable design or remove.Either make
checkForUpdateaccept a client/base URL so the mock server is used, or delete the dead test.
225-245: Timeout test isn’t tied to production client.This asserts a locally constructed
http.Client, not the one used in prod. Expose a constructor (e.g.,newHTTPClient()) and assert that.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
.gitignore(1 hunks)advanced_coverage_test.go(1 hunks)autorun_coverage_test.go(1 hunks)autorun_test.go(1 hunks)comprehensive_coverage_test.go(1 hunks)final_coverage_test.go(1 hunks)logging_coverage_test.go(1 hunks)logging_test.go(1 hunks)main_test.go(1 hunks)systray_functional_test.go(1 hunks)systray_integration_test.go(1 hunks)targeted_coverage_test.go(1 hunks)theme_coverage_test.go(1 hunks)tray_coverage_test.go(1 hunks)updater_coverage_test.go(1 hunks)updater_test.go(1 hunks)utils_coverage_test.go(1 hunks)utils_test.go(1 hunks)windows_native_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- logging_test.go
- .gitignore
🧰 Additional context used
🪛 GitHub Check: Lint and Format Check
utils_test.go
[failure] 172-172:
undefined: openRegistryKey (typecheck)
[failure] 132-132:
undefined: openRegistryKey (typecheck)
[failure] 13-13:
could not import golang.org/x/sys/windows/registry (-: build constraints exclude all Go files in /home/runner/go/pkg/mod/golang.org/x/sys@v0.31.0/windows/registry) (typecheck)
⏰ 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). (1)
- GitHub Check: Test on Windows
🔇 Additional comments (7)
tray_coverage_test.go (1)
12-27: LGTM: Windows-gated scaffold test that intentionally skips.Appropriate skip with a clear reason; harmless for CI and documents the dependency on systray initialization.
autorun_coverage_test.go (1)
13-41: LGTM: Basic executable-path check is correct and low-risk.Validates non-empty path and existence; good coverage without side effects.
theme_coverage_test.go (1)
3-4: Dual build tags are correct.Keeping both
//go:build windowsand// +build windowsensures compatibility across Go versions.comprehensive_coverage_test.go (2)
206-214: Version comparison tests look good.
244-266: Constant accessibility check is fine.final_coverage_test.go (1)
17-44: Goroutine start/stop pattern is acceptable.Given current constraints, this is a reasonable smoke test.
updater_test.go (1)
247-276: Command structure validation is fine and side‑effect free.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| openBrowser(tt.url) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Prevent browser side effects in advanced coverage.
Calling openBrowser() will open the default browser on Windows.
Apply this diff to mark these subtests as skipped (side-effectful paths are better covered via command-structure tests):
t.Run(tt.name, func(t *testing.T) {
- openBrowser(tt.url)
+ t.Skip("Avoid side effects in CI; validate command composition separately.")
})📝 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.
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| openBrowser(tt.url) | |
| }) | |
| } | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| t.Skip("Avoid side effects in CI; validate command composition separately.") | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In advanced_coverage_test.go around lines 39 to 44, the subtests currently call
openBrowser(tt.url) which triggers the system browser; modify each subtest to
skip instead of performing the side effect by calling t.Skip("skipping
side-effectful test; covered via command-structure tests") at the start of the
subtest function (remove or guard the openBrowser call so it is not executed).
| // TestLoggingAdvancedCoverage validates logging with diverse message formats | ||
| func TestLoggingAdvancedCoverage(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| message string | ||
| }{ | ||
| { | ||
| name: "very long message", | ||
| message: "This is a very long log message that contains a lot of text to test how the logging system handles longer messages that might exceed normal buffer sizes or cause formatting issues in the event log system when displayed to administrators", | ||
| }, | ||
| { | ||
| name: "message with unicode characters", | ||
| message: "Test with unicode: ñáéíóú αβγδε 中文测试 🔧⚙️", | ||
| }, | ||
| { | ||
| name: "message with newlines", | ||
| message: "Line 1\nLine 2\nLine 3", | ||
| }, | ||
| { | ||
| name: "message with tabs and spaces", | ||
| message: "Tabbed\ttext\twith multiple spaces", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| logToConsole(eventlog.Info, tt.message) | ||
| logToConsole(eventlog.Warning, tt.message) | ||
| logToConsole(eventlog.Error, tt.message) | ||
| showError(tt.message) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Gate event log/UI side-effect tests behind -short.
Logging and showError may write to Windows Event Log/GUI; avoid in default CI runs.
Apply this diff at function start:
func TestLoggingAdvancedCoverage(t *testing.T) {
+ if testing.Short() {
+ t.Skip("Skipping side-effectful logging tests in short mode")
+ }📝 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.
| // TestLoggingAdvancedCoverage validates logging with diverse message formats | |
| func TestLoggingAdvancedCoverage(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| message string | |
| }{ | |
| { | |
| name: "very long message", | |
| message: "This is a very long log message that contains a lot of text to test how the logging system handles longer messages that might exceed normal buffer sizes or cause formatting issues in the event log system when displayed to administrators", | |
| }, | |
| { | |
| name: "message with unicode characters", | |
| message: "Test with unicode: ñáéíóú αβγδε 中文测试 🔧⚙️", | |
| }, | |
| { | |
| name: "message with newlines", | |
| message: "Line 1\nLine 2\nLine 3", | |
| }, | |
| { | |
| name: "message with tabs and spaces", | |
| message: "Tabbed\ttext\twith multiple spaces", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| logToConsole(eventlog.Info, tt.message) | |
| logToConsole(eventlog.Warning, tt.message) | |
| logToConsole(eventlog.Error, tt.message) | |
| showError(tt.message) | |
| }) | |
| } | |
| } | |
| // TestLoggingAdvancedCoverage validates logging with diverse message formats | |
| func TestLoggingAdvancedCoverage(t *testing.T) { | |
| if testing.Short() { | |
| t.Skip("Skipping side-effectful logging tests in short mode") | |
| } | |
| tests := []struct { | |
| name string | |
| message string | |
| }{ | |
| { | |
| name: "very long message", | |
| message: "This is a very long log message that contains a lot of text to test how the logging system handles longer messages that might exceed normal buffer sizes or cause formatting issues in the event log system when displayed to administrators", | |
| }, | |
| { | |
| name: "message with unicode characters", | |
| message: "Test with unicode: ñáéíóú αβγδε 中文测试 🔧⚙️", | |
| }, | |
| { | |
| name: "message with newlines", | |
| message: "Line 1\nLine 2\nLine 3", | |
| }, | |
| { | |
| name: "message with tabs and spaces", | |
| message: "Tabbed\ttext\twith multiple spaces", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| logToConsole(eventlog.Info, tt.message) | |
| logToConsole(eventlog.Warning, tt.message) | |
| logToConsole(eventlog.Error, tt.message) | |
| showError(tt.message) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In advanced_coverage_test.go around lines 46 to 78, the test writes to the
Windows Event Log / GUI and must be skipped during short CI runs; at the very
start of TestLoggingAdvancedCoverage check testing.Short() and call t.Skip with
a clear message (e.g., "skipping event log/UI side-effect tests in short mode")
so the body of the test is not executed in short mode.
| // TestRegistryAdvancedOperations validates sequential registry operations | ||
| func TestRegistryAdvancedOperations(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| }{ | ||
| { | ||
| name: "multiple sequential registry operations", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, err1 := getCurrentAppThemeMode() | ||
| _ = err1 | ||
|
|
||
| err2 := setBothThemeModes(true) | ||
| _ = err2 | ||
|
|
||
| err3 := setBothThemeModes(false) | ||
| _ = err3 | ||
|
|
||
| _, err4 := getCurrentAppThemeMode() | ||
| _ = err4 | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Theme/registry mutations should be opt-in.
setBothThemeModes() touches user/theme registry; don’t run by default.
Apply this diff at function start:
func TestRegistryAdvancedOperations(t *testing.T) {
+ if testing.Short() {
+ t.Skip("Skipping registry-mutating tests in short mode")
+ }📝 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.
| // TestRegistryAdvancedOperations validates sequential registry operations | |
| func TestRegistryAdvancedOperations(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| }{ | |
| { | |
| name: "multiple sequential registry operations", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| _, err1 := getCurrentAppThemeMode() | |
| _ = err1 | |
| err2 := setBothThemeModes(true) | |
| _ = err2 | |
| err3 := setBothThemeModes(false) | |
| _ = err3 | |
| _, err4 := getCurrentAppThemeMode() | |
| _ = err4 | |
| }) | |
| } | |
| } | |
| // TestRegistryAdvancedOperations validates sequential registry operations | |
| func TestRegistryAdvancedOperations(t *testing.T) { | |
| if testing.Short() { | |
| t.Skip("Skipping registry-mutating tests in short mode") | |
| } | |
| tests := []struct { | |
| name string | |
| }{ | |
| { | |
| name: "multiple sequential registry operations", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| _, err1 := getCurrentAppThemeMode() | |
| _ = err1 | |
| err2 := setBothThemeModes(true) | |
| _ = err2 | |
| err3 := setBothThemeModes(false) | |
| _ = err3 | |
| _, err4 := getCurrentAppThemeMode() | |
| _ = err4 | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In advanced_coverage_test.go around lines 80 to 105, the test runs
registry-mutating calls by default; make these mutations opt-in by adding an
early guard at the start of TestRegistryAdvancedOperations that checks an opt-in
flag (for example an environment variable like ENABLE_REGISTRY_MUTATIONS or a
test-specific flag) and calls t.Skip or returns if the flag is not set, so the
subsequent get/setTheme registry calls are only executed when the opt-in is
explicitly provided.
| // TestOpenBrowserComprehensiveCoverage validates browser opening functionality | ||
| func TestOpenBrowserComprehensiveCoverage(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| url string | ||
| }{ | ||
| { | ||
| name: "empty URL", | ||
| url: "", | ||
| }, | ||
| { | ||
| name: "valid URL", | ||
| url: "https://example.com", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| openBrowser(tt.url) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
openBrowser causes side effects on CI.
Launching browsers in tests is non-hermetic. Inject an opener function (or wrap exec.Command) and assert invocation; otherwise skip under -short.
t.Run(tt.name, func(t *testing.T) {
- openBrowser(tt.url)
+ if testing.Short() {
+ t.Skip("skipping browser side effects in -short")
+ }
+ openBrowser(tt.url)
})📝 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.
| // TestOpenBrowserComprehensiveCoverage validates browser opening functionality | |
| func TestOpenBrowserComprehensiveCoverage(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| url string | |
| }{ | |
| { | |
| name: "empty URL", | |
| url: "", | |
| }, | |
| { | |
| name: "valid URL", | |
| url: "https://example.com", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| openBrowser(tt.url) | |
| }) | |
| } | |
| } | |
| // TestOpenBrowserComprehensiveCoverage validates browser opening functionality | |
| func TestOpenBrowserComprehensiveCoverage(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| url string | |
| }{ | |
| { | |
| name: "empty URL", | |
| url: "", | |
| }, | |
| { | |
| name: "valid URL", | |
| url: "https://example.com", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| if testing.Short() { | |
| t.Skip("skipping browser side effects in -short") | |
| } | |
| openBrowser(tt.url) | |
| }) | |
| } | |
| } |
🤖 Prompt for AI Agents
In comprehensive_coverage_test.go around lines 85 to 106, the test calls
openBrowser which launches a real browser and causes side effects on CI; change
openBrowser to be injectable (e.g. accept an opener function argument or use a
package-level var like var browserOpener func(string) error defaulting to
exec.Command logic) so tests can supply a mock that records the URL and returns
nil, update the test to set that mock before calling openBrowser and assert the
mock was invoked with the expected URL instead of launching a browser, and add a
guard to skip the test when testing.Short() is true (t.Skip) to avoid running it
in short/CI runs.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| mode, err := getCurrentAppThemeMode() | ||
| if err == nil { | ||
| if mode != 0 && mode != 1 { | ||
| t.Errorf("Invalid theme mode: %d", mode) | ||
| } | ||
| } | ||
|
|
||
| err = setBothThemeModes(true) | ||
| _ = err | ||
|
|
||
| err = setBothThemeModes(false) | ||
| _ = err | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Registry mutation in unit test.
Mock registry operations or skip under -short.
🤖 Prompt for AI Agents
In comprehensive_coverage_test.go around lines 118 to 134, the test mutates
system registry via setBothThemeModes; change it to avoid real registry
mutations by either (1) mocking the registry operations used by
setBothThemeModes/getCurrentAppThemeMode (inject a fake registry client or use
an interface and pass a test double) and exercise the logic against the mock, or
(2) skip the test when running in short mode by checking testing.Short() at the
start of the subtest and calling t.Skipf with a clear reason; ensure any
real-registry calls are removed from the test path and that the mocked
implementation simulates success/failure cases the test expects.
| // Test that openBrowser doesn't panic | ||
| // We can't easily test the actual browser opening without side effects | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| t.Errorf("openBrowser() panicked with URL %s: %v", tt.url, r) | ||
| } | ||
| }() | ||
|
|
||
| // Execute the function - it will try to open browser but we can test the code path | ||
| openBrowser(tt.url) | ||
| }) |
There was a problem hiding this comment.
Avoid launching the real browser during tests.
openBrowser() on Windows will spawn the default browser, causing flaky CI and side effects.
Apply this diff to turn the subtest into a no-side-effects check:
- // Execute the function - it will try to open browser but we can test the code path
- openBrowser(tt.url)
+ // Avoid side effects in CI; command composition is validated elsewhere.
+ t.Skip("Skipping to avoid opening a real browser; see TestRundll32CommandStructure for validation.")📝 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.
| // Test that openBrowser doesn't panic | |
| // We can't easily test the actual browser opening without side effects | |
| defer func() { | |
| if r := recover(); r != nil { | |
| t.Errorf("openBrowser() panicked with URL %s: %v", tt.url, r) | |
| } | |
| }() | |
| // Execute the function - it will try to open browser but we can test the code path | |
| openBrowser(tt.url) | |
| }) | |
| // Test that openBrowser doesn't panic | |
| // We can't easily test the actual browser opening without side effects | |
| defer func() { | |
| if r := recover(); r != nil { | |
| t.Errorf("openBrowser() panicked with URL %s: %v", tt.url, r) | |
| } | |
| }() | |
| // Avoid side effects in CI; command composition is validated elsewhere. | |
| t.Skip("Skipping to avoid opening a real browser; see TestRundll32CommandStructure for validation.") | |
| }) |
🤖 Prompt for AI Agents
In utils_coverage_test.go around lines 38-48, the subtest currently calls
openBrowser(tt.url) which launches the real browser on Windows and causes flaky
CI; update the test to avoid side effects by stubbing the browser opener:
refactor/openBrowser to call a package-level function variable (e.g., var
browserOpen = func(url string) { ... }) if not already, then in the test
temporarily replace that variable with a no-op implementation (save the
original, set noop, defer restore) and call openBrowser(tt.url); this lets you
exercise the code path and panic protection without actually spawning a browser.
| // TestWindowsNativeFunctionality validates Windows-specific API interactions under various permission scenarios | ||
| func TestWindowsNativeFunctionality(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| test func(*testing.T) | ||
| }{ | ||
| { | ||
| name: "setBothThemeModes_actual_registry_operations", | ||
| test: func(t *testing.T) { | ||
| originalMode, err := getCurrentAppThemeMode() | ||
| if err != nil { | ||
| t.Logf("Could not read current theme mode: %v", err) | ||
| return | ||
| } | ||
|
|
||
| err = setBothThemeModes(true) | ||
| if err != nil { | ||
| t.Logf("Could not set light mode: %v", err) | ||
| } | ||
|
|
||
| err = setBothThemeModes(false) | ||
| if err != nil { | ||
| t.Logf("Could not set dark mode: %v", err) | ||
| } | ||
|
|
||
| // Restore original theme mode | ||
| if originalMode == 1 { | ||
| setBothThemeModes(true) | ||
| } else { | ||
| setBothThemeModes(false) | ||
| } | ||
| }, | ||
| }, | ||
| { | ||
| name: "toggleSingleTheme_registry_operations", | ||
| test: func(t *testing.T) { | ||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| t.Logf("toggleSingleTheme panicked as expected: %v", r) | ||
| } | ||
| }() | ||
|
|
||
| app := NewApp("registry-test") | ||
| app.toggleSingleTheme(regValAppsUseLight) | ||
| app.toggleSingleTheme(regValSystemUsesLight) | ||
| }, | ||
| }, | ||
| { | ||
| name: "eventLogSourceExists_windows_check", | ||
| test: func(t *testing.T) { | ||
| exists := eventLogSourceExists() | ||
| t.Logf("Event log source exists: %v", exists) | ||
|
|
||
| // Verify consistent behavior across multiple calls | ||
| exists2 := eventLogSourceExists() | ||
| exists3 := eventLogSourceExists() | ||
| _ = exists2 | ||
| _ = exists3 | ||
| }, | ||
| }, | ||
| { | ||
| name: "installEventLogSource_windows_attempt", | ||
| test: func(t *testing.T) { | ||
| err1 := installEventLogSource() | ||
| t.Logf("Install attempt 1 result: %v", err1) | ||
|
|
||
| err2 := installEventLogSource() | ||
| t.Logf("Install attempt 2 result: %v", err2) | ||
|
|
||
| err3 := installEventLogSource() | ||
| t.Logf("Install attempt 3 result: %v", err3) | ||
| }, | ||
| }, | ||
| { | ||
| name: "isAdmin_windows_check", | ||
| test: func(t *testing.T) { | ||
| admin := isAdmin() | ||
| t.Logf("Running as admin: %v", admin) | ||
|
|
||
| // Verify consistent behavior across multiple calls | ||
| admin2 := isAdmin() | ||
| admin3 := isAdmin() | ||
| _ = admin2 | ||
| _ = admin3 | ||
| }, | ||
| }, | ||
| { | ||
| name: "logEvent_actual_windows_logging", | ||
| test: func(t *testing.T) { | ||
| logEvent(eventlog.Info, "Test info message from unit test") | ||
| logEvent(eventlog.Warning, "Test warning message from unit test") | ||
| logEvent(eventlog.Error, "Test error message from unit test") | ||
|
|
||
| // Test Unicode and extended length handling | ||
| logEvent(eventlog.Info, "Unicode test: ñáéíóú αβγδε 中文") | ||
| logEvent(eventlog.Warning, "Long message: "+strings.Repeat("A", 500)) | ||
| }, | ||
| }, | ||
| { | ||
| name: "openBrowser_windows_execution", | ||
| test: func(t *testing.T) { | ||
| openBrowser("https://example.com") | ||
|
|
||
| // Test various URL formats and edge cases | ||
| openBrowser("") | ||
| openBrowser("invalid-url-format") | ||
| openBrowser("http://") | ||
| openBrowser("https://example.com/path?param=value&other=test") | ||
| }, | ||
| }, | ||
| { | ||
| name: "withThemeRegistry_error_scenarios", | ||
| test: func(t *testing.T) { | ||
| // Test registry operations with different permission levels | ||
| err1 := withThemeRegistry(registry.QUERY_VALUE, func(key registry.Key) error { | ||
| _, _, err := key.GetIntegerValue(regValAppsUseLight) | ||
| return err | ||
| }) | ||
| t.Logf("Query value result: %v", err1) | ||
|
|
||
| err2 := withThemeRegistry(registry.SET_VALUE, func(key registry.Key) error { | ||
| return key.SetDWordValue(regValAppsUseLight, 1) | ||
| }) | ||
| t.Logf("Set value result: %v", err2) | ||
|
|
||
| err3 := withThemeRegistry(registry.READ, func(key registry.Key) error { | ||
| _, _, err := key.GetIntegerValue(regValSystemUsesLight) | ||
| return err | ||
| }) | ||
| t.Logf("Read access result: %v", err3) | ||
| }, | ||
| }, | ||
| { | ||
| name: "getExePath_windows_executable", | ||
| test: func(t *testing.T) { | ||
| path1, err1 := getExePath() | ||
| t.Logf("Executable path: %s, error: %v", path1, err1) | ||
|
|
||
| // Verify consistent behavior across multiple calls | ||
| path2, err2 := getExePath() | ||
| path3, err3 := getExePath() | ||
| _ = path2 | ||
| _ = path3 | ||
| _ = err2 | ||
| _ = err3 | ||
| }, | ||
| }, | ||
| { | ||
| name: "autorun_registry_operations", | ||
| test: func(t *testing.T) { | ||
| err1 := withAutorunRegistry(registry.QUERY_VALUE, func(key registry.Key) error { | ||
| enabled1 := isAutorunEnabled(key) | ||
| enabled2 := isAutorunEnabled(key) | ||
| enabled3 := isAutorunEnabled(key) | ||
| t.Logf("Autorun enabled checks: %v, %v, %v", enabled1, enabled2, enabled3) | ||
| return nil | ||
| }) | ||
| t.Logf("Autorun query result: %v", err1) | ||
|
|
||
| err2 := withAutorunRegistry(registry.SET_VALUE, func(key registry.Key) error { | ||
| exePath, err := getExePath() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return key.SetStringValue("WinGoDarkTray", exePath) | ||
| }) | ||
| t.Logf("Autorun set result: %v", err2) | ||
| }, | ||
| }, | ||
| { | ||
| name: "showNotificationWithFallback_windows_notifications", | ||
| test: func(t *testing.T) { | ||
| showNotificationWithFallback("Test Title", "Test message from unit test", "Test tooltip") | ||
| showNotificationWithFallback("Unicode Test", "Testing ñáéíóú αβγδε 中文", "Unicode tooltip") | ||
| showNotificationWithFallback("", "", "") | ||
| showNotificationWithFallback("Long Title", string(make([]byte, 200)), "Long tooltip") | ||
|
|
||
| // Allow notification processing time | ||
| time.Sleep(100 * time.Millisecond) | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, tt.test) | ||
| } | ||
| } |
There was a problem hiding this comment.
Run Windows-native, side-effectful tests only when explicitly allowed.
This suite toggles theme, opens browser, touches event log, etc. Gate the entire test to avoid CI/user side effects.
func TestWindowsNativeFunctionality(t *testing.T) {
+ if os.Getenv("ALLOW_WINDOWS_E2E") != "1" {
+ t.Skip("Skipping Windows-native E2E; set ALLOW_WINDOWS_E2E=1 to run.")
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In windows_native_test.go around lines 18 to 204, the test suite runs
side-effectful Windows-native operations unconditionally; gate the entire
TestWindowsNativeFunctionality so it only runs when explicitly allowed (e.g., an
environment variable or test flag) and only on Windows. Add a short early check
at the top of the test that: 1) returns t.Skipf(...) unless runtime.GOOS ==
"windows"; 2) reads an explicit opt-in (for example
os.Getenv("RUN_WINDOWS_NATIVE_TESTS") == "1" or a flag like -run_windows_native)
and calls t.Skipf(...) when not set; and 3) document the opt-in method in the
test comment so CI and developers must opt in before side effects occur.
| name: "openBrowser_windows_execution", | ||
| test: func(t *testing.T) { | ||
| openBrowser("https://example.com") | ||
|
|
||
| // Test various URL formats and edge cases | ||
| openBrowser("") | ||
| openBrowser("invalid-url-format") | ||
| openBrowser("http://") | ||
| openBrowser("https://example.com/path?param=value&other=test") | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Avoid launching a browser in tests.
openBrowser will spawn external processes. Skip by default or mock.
- test: func(t *testing.T) {
+ test: func(t *testing.T) {
+ if os.Getenv("ALLOW_WINDOWS_E2E") != "1" {
+ t.Skip("Skipping browser launch; set ALLOW_WINDOWS_E2E=1 to run.")
+ }
openBrowser("https://example.com")
...📝 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.
| name: "openBrowser_windows_execution", | |
| test: func(t *testing.T) { | |
| openBrowser("https://example.com") | |
| // Test various URL formats and edge cases | |
| openBrowser("") | |
| openBrowser("invalid-url-format") | |
| openBrowser("http://") | |
| openBrowser("https://example.com/path?param=value&other=test") | |
| }, | |
| }, | |
| name: "openBrowser_windows_execution", | |
| test: func(t *testing.T) { | |
| if os.Getenv("ALLOW_WINDOWS_E2E") != "1" { | |
| t.Skip("Skipping browser launch; set ALLOW_WINDOWS_E2E=1 to run.") | |
| } | |
| openBrowser("https://example.com") | |
| // Test various URL formats and edge cases | |
| openBrowser("") | |
| openBrowser("invalid-url-format") | |
| openBrowser("http://") | |
| openBrowser("https://example.com/path?param=value&other=test") | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In windows_native_test.go around lines 117 to 127, the test calls openBrowser
which spawns external processes; change the test to avoid launching a real
browser by either (A) skipping it by default (e.g., t.Skip("skipping external
process test") or gate with testing.Short()/env var) or (B) refactor openBrowser
to use an injectable command-runner or function variable and in this test
replace that runner with a no-op/mock that records the intended command and
arguments; update assertions to verify the mock was invoked with expected URLs
instead of actually launching a browser.
| name: "autorun_registry_operations", | ||
| test: func(t *testing.T) { | ||
| err1 := withAutorunRegistry(registry.QUERY_VALUE, func(key registry.Key) error { | ||
| enabled1 := isAutorunEnabled(key) | ||
| enabled2 := isAutorunEnabled(key) | ||
| enabled3 := isAutorunEnabled(key) | ||
| t.Logf("Autorun enabled checks: %v, %v, %v", enabled1, enabled2, enabled3) | ||
| return nil | ||
| }) | ||
| t.Logf("Autorun query result: %v", err1) | ||
|
|
||
| err2 := withAutorunRegistry(registry.SET_VALUE, func(key registry.Key) error { | ||
| exePath, err := getExePath() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return key.SetStringValue("WinGoDarkTray", exePath) | ||
| }) | ||
| t.Logf("Autorun set result: %v", err2) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Do not write to the real Autorun registry key in tests.
This persists system state (app starts on login). Replace with a temp HKCU subkey and validate via isAutorunEnabled(key).
-{
- name: "autorun_registry_operations",
- test: func(t *testing.T) {
- err1 := withAutorunRegistry(registry.QUERY_VALUE, func(key registry.Key) error {
- enabled1 := isAutorunEnabled(key)
- enabled2 := isAutorunEnabled(key)
- enabled3 := isAutorunEnabled(key)
- t.Logf("Autorun enabled checks: %v, %v, %v", enabled1, enabled2, enabled3)
- return nil
- })
- t.Logf("Autorun query result: %v", err1)
-
- err2 := withAutorunRegistry(registry.SET_VALUE, func(key registry.Key) error {
- exePath, err := getExePath()
- if err != nil { return err }
- return key.SetStringValue("WinGoDarkTray", exePath)
- })
- t.Logf("Autorun set result: %v", err2)
- },
-},
+{
+ name: "autorun_registry_operations (temp key)",
+ test: func(t *testing.T) {
+ keyPath := fmt.Sprintf("Software\\WinGoDarkTrayTestAutorun_%d", os.Getpid())
+ testKey, _, err := registry.CreateKey(registry.CURRENT_USER, keyPath, registry.ALL_ACCESS)
+ if err != nil { t.Skipf("Cannot create test registry key: %v", err) }
+ defer func() {
+ testKey.Close()
+ registry.DeleteKey(registry.CURRENT_USER, keyPath)
+ }()
+ // Write and verify using the temp key only.
+ exePath, _ := getExePath()
+ _ = testKey.SetStringValue("WinGoDarkTray", exePath)
+ if !isAutorunEnabled(testKey) {
+ t.Errorf("expected autorun enabled for temp key")
+ }
+ _ = testKey.DeleteValue("WinGoDarkTray")
+ if isAutorunEnabled(testKey) {
+ t.Errorf("expected autorun disabled after deletion")
+ }
+ },
+},📝 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.
| name: "autorun_registry_operations", | |
| test: func(t *testing.T) { | |
| err1 := withAutorunRegistry(registry.QUERY_VALUE, func(key registry.Key) error { | |
| enabled1 := isAutorunEnabled(key) | |
| enabled2 := isAutorunEnabled(key) | |
| enabled3 := isAutorunEnabled(key) | |
| t.Logf("Autorun enabled checks: %v, %v, %v", enabled1, enabled2, enabled3) | |
| return nil | |
| }) | |
| t.Logf("Autorun query result: %v", err1) | |
| err2 := withAutorunRegistry(registry.SET_VALUE, func(key registry.Key) error { | |
| exePath, err := getExePath() | |
| if err != nil { | |
| return err | |
| } | |
| return key.SetStringValue("WinGoDarkTray", exePath) | |
| }) | |
| t.Logf("Autorun set result: %v", err2) | |
| }, | |
| }, | |
| { | |
| name: "autorun_registry_operations (temp key)", | |
| test: func(t *testing.T) { | |
| keyPath := fmt.Sprintf("Software\\WinGoDarkTrayTestAutorun_%d", os.Getpid()) | |
| testKey, _, err := registry.CreateKey(registry.CURRENT_USER, keyPath, registry.ALL_ACCESS) | |
| if err != nil { t.Skipf("Cannot create test registry key: %v", err) } | |
| defer func() { | |
| testKey.Close() | |
| registry.DeleteKey(registry.CURRENT_USER, keyPath) | |
| }() | |
| // Write and verify using the temp key only. | |
| exePath, _ := getExePath() | |
| _ = testKey.SetStringValue("WinGoDarkTray", exePath) | |
| if !isAutorunEnabled(testKey) { | |
| t.Errorf("expected autorun enabled for temp key") | |
| } | |
| _ = testKey.DeleteValue("WinGoDarkTray") | |
| if isAutorunEnabled(testKey) { | |
| t.Errorf("expected autorun disabled after deletion") | |
| } | |
| }, | |
| }, |
🤖 Prompt for AI Agents
In windows_native_test.go around lines 166 to 186, the test currently writes to
the real Autorun registry key; instead create a temporary HKCU subkey, use that
subkey for both the query and set operations, and clean it up afterwards.
Specifically: open registry.CURRENT_USER and call registry.CreateKey with a
unique test path (e.g. "Software\\<testname>-<random>" or based on t.Name()),
defer deleting that key, pass the created key into your isAutorunEnabled checks
and SetStringValue calls (or adapt the helper to accept a key) so the test
validates behavior against the temp subkey only and does not modify the real
autorun entry.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
updater.go (1)
49-80: Consider propagating context through HTTP client.While the function accepts context, it doesn't use it for the HTTP request cancellation. Consider using
http.NewRequestWithContextfor better cancellation support.Apply this diff to properly use context for HTTP request cancellation:
func checkForUpdateWithContext(ctx context.Context, currentVersion string, updateNowItem *systray.MenuItem) { client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(githubReleasesAPI) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, githubReleasesAPI, nil) + if err != nil { + logEvent(eventlog.Error, "Failed to create request: "+err.Error()) + return + } + resp, err := client.Do(req) if err != nil { logEvent(eventlog.Error, "Failed to fetch latest release: "+err.Error()) return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
logging.go(2 hunks)main.go(2 hunks)systray_functional_test.go(1 hunks)tray_handlers.go(1 hunks)updater.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- main.go
🧰 Additional context used
🧬 Code graph analysis (2)
tray_handlers.go (1)
main.go (1)
App(16-22)
systray_functional_test.go (1)
main.go (1)
NewApp(25-32)
🪛 GitHub Check: Test on Windows
systray_functional_test.go
[failure] 10-10:
"sync" imported and not used
🪛 GitHub Actions: Test Suite
systray_functional_test.go
[error] 1-1: gofmt formatting check failed. Run 'gofmt -s -w .' to fix code style issues in this file.
🔇 Additional comments (14)
logging.go (3)
1-4: Good addition of Windows build constraints!The build tags correctly restrict this file to Windows compilation, aligning with the Windows-specific functionality.
131-136: Excellent defensive programming for CI environments!The panic recovery wrapper properly handles systray operations that may fail in headless/CI environments, with appropriate error logging that distinguishes these expected failures.
140-145: LGTM! Consistent error handling in the goroutine.The deferred recovery in the goroutine ensures that tooltip reset failures are also handled gracefully in CI environments.
updater.go (4)
1-4: Good Windows-specific gating!The build constraints properly restrict the updater functionality to Windows.
25-27: Clean delegation to context-aware variant.The wrapper function maintains backward compatibility while enabling context-aware behavior.
29-43: Well-structured context-aware update handler!The function properly handles context cancellation, enabling graceful shutdown and better testability. The select cases are correctly ordered.
45-47: Good backward compatibility wrapper!Maintains existing API while delegating to the context-aware implementation.
tray_handlers.go (3)
1-4: Proper Windows build constraints!The file is correctly gated for Windows-only compilation.
14-16: Clean backward compatibility wrapper!The function maintains the existing API while enabling context-aware behavior.
18-35: Well-implemented context-aware menu handler!The function properly handles context cancellation as the first case in the select statement, ensuring timely shutdown. All menu item click channels are correctly handled.
systray_functional_test.go (4)
18-86: Make systray E2E tests opt-in to prevent CI issues.These tests can block or panic in headless CI environments. They should be opt-in via environment variable.
89-178: Make systray-dependent subtests opt-in.These tests deliberately induce panics and can hang without a tray. They should be gated behind an environment variable.
181-249: Make initialization tests opt-in to avoid goroutine leaks.The startEventHandlers function spawns goroutines that can leak in test environments.
251-309: Make real systray initialization opt-in.This test attempts real systray UI initialization which can open UI windows and affect the OS.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (17)
main_test.go (6)
3-4: Build tags are fine; consider dropping legacy tag (optional).You have both
//go:build windowsand// +build windows. Keeping both is OK for Go <1.17 compatibility. If you only target Go 1.17+, you can drop the legacy+buildline.
12-27: Test adds no coverage; remove or mark as integration.This test always skips and validates nothing. Prefer removing it or moving a real check under an
integrationbuild tag.Apply this diff to remove the no-op test:
-func TestMainFunctionStructure(t *testing.T) { - tests := []struct { - name string - }{ - { - name: "main function initialization", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Skip actual main execution as it would start the full application - t.Skip("main() function starts full application - skipping to avoid side effects") - }) - } -}
29-44: onExit is testable in the same package; make this a real test.Since this file uses
package main, you can call unexportedonExit()directly. Replace the skip with a panic-safety check.Apply this diff:
-func TestOnExitFunction(t *testing.T) { - tests := []struct { - name string - }{ - { - name: "onExit function call", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Skip this test as onExit is not exported - t.Skip("onExit function is not exported - cannot test directly") - }) - } -} +func TestOnExitFunction(t *testing.T) { + t.Run("onExit does not panic", func(t *testing.T) { + defer func() { + if r := recover(); r != nil { + t.Fatalf("onExit panicked: %v", r) + } + }() + onExit() + }) +}
46-70: Skip-only scaffolding; convert to integration or delete.These subtests always skip and assert nothing. Either (a) move them to
main_integration_test.gowith//go:build windows && integration, or (b) delete until you can inject dependencies to unit-test them without systray.I can sketch a small dependency-injection seam (wrapping systray calls) so these can be unit tested—say the word.
72-87: Menu creation test should be integration or DI-backed.As written it always skips. If you wrap systray calls behind an interface and inject a fake in tests, you can assert items/ordering without starting systray. Otherwise, gate it under an
integrationtag.
89-104: Event handler test needs cancellation/DI; otherwise mark as integration.Introduce context-driven handlers or injectable channels so tests can run deterministically and exit cleanly. Without that, keep as an integration test.
advanced_coverage_test.go (1)
283-294: Use registry constants instead of magic numbers; fix duplicate access.Both cases currently use 0x20019; use QUERY_VALUE for query and READ for read.
Apply this diff:
tests := []struct { name string keyPath string access uint32 }{ { name: "query access to theme registry", keyPath: `Software\Microsoft\Windows\CurrentVersion\Themes\Personalize`, - access: 0x20019, + access: registry.QUERY_VALUE, }, { name: "read access to theme registry", keyPath: `Software\Microsoft\Windows\CurrentVersion\Themes\Personalize`, - access: 0x20019, + access: registry.READ, }, }updater_test.go (2)
99-112: Skip before spinning up the mock server.You create httptest.Server and then skip; move t.Skip earlier to avoid resource use.
Apply this diff:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Create mock server - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(tt.statusCode) - if tt.mockResponse != nil { - json.NewEncoder(w).Encode(tt.mockResponse) - } - })) - defer server.Close() - - // Skip this test as it requires systray initialization and modifies global state - t.Skip("checkForUpdate requires systray initialization and modifies global state") + t.Skip("checkForUpdate requires systray initialization and modifies global state") + // The code below intentionally not executed when skipped. + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tt.statusCode) + if tt.mockResponse != nil { + json.NewEncoder(w).Encode(tt.mockResponse) + } + })) + defer server.Close() }) }
43-114: Make checkForUpdate testable by injecting deps.Refactor production checkForUpdate to accept an http.Client and base URL (or a ReleaseFetcher interface) so this test can actually hit httptest.Server without systray/global state.
I can sketch the interface and updates if you want.
theme_test.go (2)
8-12: Import os for opt‑in gating.Apply this diff:
import ( + "os" "testing" "golang.org/x/sys/windows/registry" )
96-111: Gate registry reads under -short.Even reads can fail on CI; skip in short mode.
Apply this diff:
func TestGetCurrentAppThemeMode(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry access in -short mode") + } // This test attempts to read the actual theme registrywindows_native_test.go (2)
207-211: Gate error‑path registry tests behind opt‑in too.Apply this diff:
func TestWindowsErrorPaths(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping Windows registry permission tests; set ALLOW_WINDOWS_E2E=1 to run.") + }
279-283: Gate logging flood tests behind opt‑in.Apply this diff:
func TestWindowsAPISafeInteractions(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping verbose Windows API interactions in short/CI runs.") + }targeted_coverage_test.go (3)
8-13: Import os for gating.Apply this diff:
import ( + "os" "testing" "time" "github.com/getlantern/systray" )
178-186: Also gate setupEventLog extended tests.Apply this diff:
func TestSetupEventLogExtended(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("requires Windows event log; set ALLOW_WINDOWS_E2E=1 to run.") + }
272-297: Gate direct registry operations.Apply this diff:
func TestRegistryErrorScenarios(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry access in -short mode") + }systray_functional_test.go (1)
8-14: Import os for opt‑in gating.Apply this diff:
import ( "context" + "os" "testing" "time" "github.com/getlantern/systray" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
advanced_coverage_test.go(1 hunks)autorun_coverage_test.go(1 hunks)autorun_test.go(1 hunks)comprehensive_coverage_test.go(1 hunks)final_coverage_test.go(1 hunks)logging_coverage_test.go(1 hunks)logging_test.go(1 hunks)main_test.go(1 hunks)systray_functional_test.go(1 hunks)systray_integration_test.go(1 hunks)targeted_coverage_test.go(1 hunks)theme_coverage_test.go(1 hunks)theme_test.go(1 hunks)tray_coverage_test.go(1 hunks)tray_handlers_test.go(1 hunks)updater_coverage_test.go(1 hunks)updater_test.go(1 hunks)utils_coverage_test.go(1 hunks)windows_native_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- logging_test.go
- tray_handlers_test.go
- utils_coverage_test.go
- tray_coverage_test.go
- updater_coverage_test.go
- comprehensive_coverage_test.go
- autorun_coverage_test.go
- autorun_test.go
- systray_integration_test.go
- final_coverage_test.go
- logging_coverage_test.go
🧰 Additional context used
🧬 Code graph analysis (4)
updater_test.go (2)
updater.go (3)
runUpdateCommand(75-81)checkForUpdate(32-63)showUpdateNotification(66-73)semver_test.go (1)
TestIsVersionNewer(5-38)
theme_test.go (1)
theme.go (8)
a(111-147)a(84-109)setBothThemeModes(38-52)a(153-155)withThemeRegistry(17-24)a(149-151)getCurrentAppThemeMode(27-35)a(54-81)
main_test.go (1)
main.go (7)
main(31-34)a(38-47)a(62-85)a(92-103)toggleSystemItem(10-16)onExit(36-36)a(49-52)
systray_functional_test.go (1)
main.go (1)
NewApp(25-32)
⏰ 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). (2)
- GitHub Check: CodeQL Analysis
- GitHub Check: Test on Windows
🔇 Additional comments (16)
main_test.go (1)
106-143: Use the real constructor (NewApp) in tests — verify it has no systray side effects.Don't re-implement version logic in the test; call NewApp(tt.buildVersion) and assert app.version, running subtests in parallel (tt := tt; t.Parallel()). I couldn't locate NewApp with the provided search and observed many tests skipping because of systray initialization — confirm NewApp exists, only sets fields (no systray or global side effects), and that the default version is "v0.0.0" before applying this change in main_test.go:106-143.
theme_test.go (2)
57-94: LGTM: withThemeRegistry happy‑path coverage.
113-156: Critical: test mutates user theme; make opt‑in.Do not write HKCU by default. Gate behind -short and an explicit env var.
Apply this diff:
func TestSetBothThemeModes(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_THEME_REG_WRITE") != "1" { + t.Skip("integration: would modify HKCU theme; set ALLOW_THEME_REG_WRITE=1 to run explicitly") + }theme_coverage_test.go (1)
116-129: Skip registry access under -short.Avoid flakiness and side effects in CI.
Apply this diff:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + if testing.Short() { + t.Skip("skipping registry access in -short mode") + } // Test getCurrentAppThemeMode error handlingadvanced_coverage_test.go (3)
39-42: Skip browser‑launching tests to avoid CI side effects.Launching the real browser in unit tests is unsafe; skip and validate command composition elsewhere.
Apply this diff:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - openBrowser(tt.url) + t.Skip("Avoid side effects in CI; validate command composition separately.") }) }
47-48: Gate event log/UI side‑effect tests behind -short.These write to event log / show GUI; skip in short runs.
Apply this diff:
func TestLoggingAdvancedCoverage(t *testing.T) { + if testing.Short() { + t.Skip("Skipping side-effectful logging tests in short mode") + }
81-82: Do not mutate registry by default; gate under -short.setBothThemeModes writes HKCU; make opt‑in.
Apply this diff:
func TestRegistryAdvancedOperations(t *testing.T) { + if testing.Short() { + t.Skip("Skipping registry-mutating tests in short mode") + }windows_native_test.go (2)
19-23: Gate entire Windows‑native E2E suite behind opt‑in.Prevents browser launches, registry/event log writes, and other side effects during CI.
Apply this diff:
func TestWindowsNativeFunctionality(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping Windows-native E2E; set ALLOW_WINDOWS_E2E=1 to run.") + }
166-186: Avoid writing to real Autorun key; use a temp HKCU subkey.Use a throwaway subkey and clean up after to avoid persisting autorun state.
Apply this diff:
- err2 := withAutorunRegistry(registry.SET_VALUE, func(key registry.Key) error { - exePath, err := getExePath() - if err != nil { - return err - } - return key.SetStringValue("WinGoDarkTray", exePath) - }) - t.Logf("Autorun set result: %v", err2) + keyPath := fmt.Sprintf("Software\\WinGoDarkTrayTestAutorun_%d", os.Getpid()) + testKey, _, err := registry.CreateKey(registry.CURRENT_USER, keyPath, registry.ALL_ACCESS) + if err != nil { + t.Skipf("Cannot create test registry key: %v", err) + } + defer func() { + testKey.Close() + _ = registry.DeleteKey(registry.CURRENT_USER, keyPath) + }() + exePath, _ := getExePath() + _ = testKey.SetStringValue("WinGoDarkTray", exePath) + if !isAutorunEnabled(testKey) { + t.Errorf("expected autorun enabled for temp key") + } + _ = testKey.DeleteValue("WinGoDarkTray") + if isAutorunEnabled(testKey) { + t.Errorf("expected autorun disabled after deletion") + }targeted_coverage_test.go (3)
16-21: Gate systray/side‑effect tests behind -short and env.These paths can panic/hang and touch OS state.
Apply this diff:
func TestTargetedCoverageImprovement(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("requires systray/Windows E2E; set ALLOW_WINDOWS_E2E=1 to run.") + }
239-271: Avoid real network/API calls in unit tests.checkForUpdate hits GitHub; inject http.Client/baseURL or skip by default.
Apply this diff:
func TestUpdaterEdgeCases(t *testing.T) { + if testing.Short() { + t.Skip("networked test; skip in unit runs") + }And consider refactoring production code to accept a client/baseURL for httptest.Server.
206-237: Do not toggle themes/registry by default.Apply this diff:
func TestThemeToggleEdgeCases(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("skipping registry mutation tests; set ALLOW_WINDOWS_E2E=1 to run.") + }systray_functional_test.go (4)
16-21: Make functional systray test opt‑in to avoid UI/hangs.Apply this diff:
func TestSystrayFunctionsWithTimeout(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping systray E2E; set ALLOW_WINDOWS_E2E=1 to run.") + } // Use a very short timeout to avoid hanging the test suite
179-184: Gate initialization paths that spawn goroutines.Apply this diff:
func TestApplicationInitialization(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping initialization E2E; set ALLOW_WINDOWS_E2E=1 to run.") + }
87-93: Gate systray‑dependent subtests.Apply this diff:
func TestSystrayDependentFunctions(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping systray-dependent tests; set ALLOW_WINDOWS_E2E=1 to run.") + }
250-254: Skip real systray UI init by default.Apply this diff:
func TestSystrayInitializationAttempt(t *testing.T) { + if testing.Short() || os.Getenv("ALLOW_WINDOWS_E2E") != "1" { + t.Skip("Skipping real systray init; set ALLOW_WINDOWS_E2E=1 to run.") + }
2 participants
Summary by CodeRabbit
New Features
CI/CD
Tests
Documentation
Chores