Skip to content

test(upgrade): fix GGA backup test for Windows paths#916

Open
decode2 wants to merge 2 commits into
Gentleman-Programming:mainfrom
decode2:fix/test-gga-windows-paths
Open

test(upgrade): fix GGA backup test for Windows paths#916
decode2 wants to merge 2 commits into
Gentleman-Programming:mainfrom
decode2:fix/test-gga-windows-paths

Conversation

@decode2

@decode2 decode2 commented Jun 17, 2026

Copy link
Copy Markdown

Summary

Fixes \TestConfigPathsForBackup_GGAExtrasAreIncluded\ which was failing on Windows because it hardcoded Unix-style paths.

Problem

The test created GGA config files at ~/.config/gga/config\ (Unix-style path), but \gga.ConfigPath(homeDir)\ uses platform-specific paths:

  • Windows: %APPDATA%\gga\config\ (e.g., \C:\Users...\AppData\Roaming\gga\config)
  • Unix: ~/.config/gga/config\

This caused the test to fail on Windows because \configPathsForBackup()\ correctly returned the Windows path, but the test expected the Unix path it had created.

Solution

Use \gga.ConfigPath(homeDir)\ and \gga.RuntimePRModePath(homeDir)\ instead of hardcoding paths. This ensures the test creates files at the correct platform-specific locations.

Changes

  • Import \github.com/gentleman-programming/gentle-ai/internal/components/gga\
  • Replace hardcoded paths with \gga.ConfigPath(homeDir)\ and \gga.RuntimePRModePath(homeDir)\

Testing

  • ✅ Test passes on Windows
  • ✅ All other tests in \internal/update/upgrade\ continue to pass

Summary by CodeRabbit

Release Notes

  • Tests
    • Improved backup-path tests for GGA extras to use platform-correct locations derived from the home directory.
    • Updated GGA injection-related tests to explicitly set Windows-style roaming environment paths via APPDATA to ensure consistent behavior across systems.

The test was hardcoding Unix-style paths (~/.config/gga/config) but
gga.ConfigPath() uses %APPDATA%\gga\config on Windows. Use the gga
package functions to get platform-correct paths instead of hardcoding.

This ensures the test works correctly on all platforms.
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8066fbdc-9344-4475-a866-c0dfba53b946

📥 Commits

Reviewing files that changed from the base of the PR and between 0740005 and 633a493.

📒 Files selected for processing (2)
  • internal/components/gga/config_test.go
  • internal/update/upgrade/executor_test.go

📝 Walkthrough

Walkthrough

Two test files are updated to ensure platform-correct GGA path handling. GGA component tests now set the APPDATA environment variable for Windows path derivation, and the backup test replaces hardcoded path strings with calls to gga.ConfigPath() and gga.RuntimePRModePath() helper functions. No exported signatures are changed.

Changes

GGA Path Helpers and Environment Setup in Tests

Layer / File(s) Summary
GGA component test environment setup
internal/components/gga/config_test.go
TestInjectWritesConfigAndAgents and TestInjectIsIdempotent now set the APPDATA environment variable to a Windows roaming path before invoking Inject(...) to enable platform-correct path derivation during tests.
Backup test migration to GGA path helpers
internal/update/upgrade/executor_test.go
Adds internal/components/gga import and updates TestConfigPathsForBackup_GGAExtrasAreIncluded to set APPDATA and compute expected GGA paths via gga.ConfigPath(homeDir) and gga.RuntimePRModePath(homeDir) instead of literal filesystem paths, keeping inclusion assertions intact.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: fixing a GGA backup test to handle Windows paths correctly by using platform-specific path functions instead of hardcoded Unix paths.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@Alan-TheGentleman Alan-TheGentleman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch on the platform-specific GGA paths, but this test still has an isolation problem on Windows.

gga.ConfigPath(homeDir) can use %APPDATA% when it is set, which means the test may write to the real user profile instead of the temp home. Tests cannot mutate a developer's actual GGA config location, even accidentally.

Please control APPDATA in the test, or use a helper/seam that guarantees the generated path stays inside t.TempDir() on Windows.

@decode2

decode2 commented Jun 23, 2026

Copy link
Copy Markdown
Author

Hi @Alan-TheGentleman, I've isolated the tests on Windows:

  • Set the \APPDATA\ environment variable to a subdirectory inside \ .TempDir()\ in the tests.
  • This guarantees that paths resolved via \gga.ConfigPath(homeDir)\ stay isolated and never mutate the actual user roaming directory.

Ready for re-review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants