Skip to content

Conversation

JAORMX
Copy link
Collaborator

@JAORMX JAORMX commented Sep 1, 2025

Problem:
Tests were modifying the real configuration file at ~/.config/toolhive/config.yaml
because they were using the global singleton config pattern, which automatically
loads and saves to the default XDG config path.

Root Cause:

  1. The singleton pattern in pkg/config was being triggered by tests
  2. LoadOrCreateConfigWithPath had a critical bug where it called config.save()
    instead of config.saveToPath(configPath), causing even tests with temp
    directories to save to the real config location
  3. Tests were directly calling config.GetConfig() which triggers singleton
    initialization with the real config file

Solution:

  1. Introduced dependency injection with config.Provider interface
    • DefaultProvider: Uses the singleton (for production)
    • PathProvider: Uses a specific path (for tests)
  2. Fixed the critical bug in LoadOrCreateConfigWithPath (line 179)
    • Changed from config.save() to config.saveToPath(configPath)
  3. Updated all components to accept config providers:
    • API routes (registry, secrets)
    • Client manager
    • Workloads manager
    • Registry factory
    • Runner components
    • Migration utilities
  4. Modified all tests to use CreateTestConfigProvider() helper
    • Creates temporary directories for test isolation
    • Uses PathProvider to avoid singleton initialization
  5. Fixed race conditions in parallel tests by ensuring each
    subtest has its own mock controller

Impact:

  • Tests no longer modify the real config file
  • Each test runs in complete isolation with its own config
  • Parallel test execution is now safe
  • Production code continues to use the singleton pattern
  • Backward compatibility maintained for existing code

Verified:
All 56 test functions in pkg/api/v1 and pkg/registry now pass
without modifying ~/.config/toolhive/config.yaml

Signed-off-by: Juan Antonio Osorio [email protected]

@JAORMX JAORMX force-pushed the fix-test-config-isolation branch from 3d83978 to 9171240 Compare September 1, 2025 18:21
@JAORMX JAORMX force-pushed the fix-test-config-isolation branch 2 times, most recently from 5c75f22 to ea851da Compare September 2, 2025 07:28
Problem:
Tests were modifying the real configuration file at ~/.config/toolhive/config.yaml
because they were using the global singleton config pattern, which automatically
loads and saves to the default XDG config path.

Root Cause:
1. The singleton pattern in pkg/config was being triggered by tests
2. LoadOrCreateConfigWithPath had a critical bug where it called config.save()
   instead of config.saveToPath(configPath), causing even tests with temp
   directories to save to the real config location
3. Tests were directly calling config.GetConfig() which triggers singleton
   initialization with the real config file

Solution:
1. Introduced dependency injection with config.Provider interface
   - DefaultProvider: Uses the singleton (for production)
   - PathProvider: Uses a specific path (for tests)
2. Fixed the critical bug in LoadOrCreateConfigWithPath (line 179)
   - Changed from config.save() to config.saveToPath(configPath)
3. Updated all components to accept config providers:
   - API routes (registry, secrets)
   - Client manager
   - Workloads manager
   - Registry factory
   - Runner components
   - Migration utilities
4. Modified all tests to use CreateTestConfigProvider() helper
   - Creates temporary directories for test isolation
   - Uses PathProvider to avoid singleton initialization
5. Fixed race conditions in parallel tests by ensuring each
   subtest has its own mock controller

Impact:
- Tests no longer modify the real config file
- Each test runs in complete isolation with its own config
- Parallel test execution is now safe
- Production code continues to use the singleton pattern
- Backward compatibility maintained for existing code

Verified:
All 56 test functions in pkg/api/v1 and pkg/registry now pass
without modifying ~/.config/toolhive/config.yaml

Signed-off-by: Juan Antonio Osorio <[email protected]>
@JAORMX JAORMX force-pushed the fix-test-config-isolation branch from ea851da to 85d8974 Compare September 2, 2025 07:36
@JAORMX JAORMX requested a review from yrobla September 2, 2025 07:36
@JAORMX JAORMX merged commit ae0fe4d into main Sep 3, 2025
16 checks passed
@JAORMX JAORMX deleted the fix-test-config-isolation branch September 3, 2025 11:38
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.

3 participants