Skip to content

Commit 6c4efe3

Browse files
JAORMXJAORMX
authored
Enable parallel test execution by eliminating XDG global state (#1634)
Replace XDG environment manipulation with dependency injection pattern. Add config.Provider interface for isolated test configurations and implement ClientManager struct for dependency injection. Enable t.Parallel() in all affected test files while maintaining backward compatibility with existing APIs. This resolves issues with tests that prevented parallel execution due to t.Setenv('XDG_CONFIG_HOME') usage which is incompatible with Go 1.24+.
1 parent fd0d0b8 commit 6c4efe3

File tree

9 files changed

+539
-282
lines changed

9 files changed

+539
-282
lines changed

‎cmd/thv/app/run_flags_test.go‎

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"path/filepath"
66
"testing"
77

8-
"github.com/adrg/xdg"
98
"github.com/spf13/cobra"
109
"github.com/stretchr/testify/assert"
1110
"github.com/stretchr/testify/require"
@@ -14,28 +13,37 @@ import (
1413
"github.com/stacklok/toolhive/pkg/logger"
1514
)
1615

17-
// mockConfig creates a temporary config file with the provided configuration.
18-
func mockConfig(t *testing.T, cfg *config.Config) func() {
16+
// createTestConfigProvider creates a config provider for testing with the provided configuration.
17+
func createTestConfigProvider(t *testing.T, cfg *config.Config) (config.Provider, func()) {
1918
t.Helper()
19+
20+
// Create a temporary directory for the test
2021
tempDir := t.TempDir()
21-
originalXDGConfigHome := os.Getenv("XDG_CONFIG_HOME")
22-
t.Setenv("XDG_CONFIG_HOME", tempDir)
23-
xdg.Reload()
22+
23+
// Create the config directory structure
2424
configDir := filepath.Join(tempDir, "toolhive")
2525
err := os.MkdirAll(configDir, 0755)
2626
require.NoError(t, err)
27+
28+
// Set up the config file path
29+
configPath := filepath.Join(configDir, "config.yaml")
30+
31+
// Create a path-based config provider
32+
provider := config.NewPathProvider(configPath)
33+
34+
// Write the config file if one is provided
2735
if cfg != nil {
28-
err = config.UpdateConfig(func(c *config.Config) { *c = *cfg })
36+
err = provider.UpdateConfig(func(c *config.Config) { *c = *cfg })
2937
require.NoError(t, err)
3038
}
31-
return func() {
32-
t.Setenv("XDG_CONFIG_HOME", originalXDGConfigHome)
33-
xdg.Reload()
39+
40+
return provider, func() {
41+
// Cleanup is handled by t.TempDir()
3442
}
3543
}
3644

37-
//nolint:paralleltest // Cannot use t.Parallel() with t.Setenv() in Go 1.24+
3845
func TestBuildRunnerConfig_TelemetryProcessing(t *testing.T) {
46+
t.Parallel()
3947
// Initialize logger to prevent nil pointer dereference
4048
logger.Initialize()
4149

@@ -152,14 +160,15 @@ func TestBuildRunnerConfig_TelemetryProcessing(t *testing.T) {
152160

153161
for _, tt := range tests {
154162
t.Run(tt.name, func(t *testing.T) {
163+
t.Parallel()
155164
cmd := &cobra.Command{}
156165
AddRunFlags(cmd, &RunFlags{})
157166
tt.setupFlags(cmd)
158-
cleanup := mockConfig(t, &config.Config{
167+
configProvider, cleanup := createTestConfigProvider(t, &config.Config{
159168
OTEL: tt.configOTEL,
160169
})
161170
defer cleanup()
162-
configInstance := config.GetConfig()
171+
configInstance := configProvider.GetConfig()
163172
finalEndpoint, finalSamplingRate, finalEnvVars := getTelemetryFromFlags(
164173
cmd,
165174
configInstance,
@@ -176,8 +185,8 @@ func TestBuildRunnerConfig_TelemetryProcessing(t *testing.T) {
176185
}
177186
}
178187

179-
//nolint:paralleltest // Cannot use t.Parallel() with t.Setenv() in Go 1.24+
180188
func TestBuildRunnerConfig_TelemetryProcessing_Integration(t *testing.T) {
189+
t.Parallel()
181190
// This is a more complete integration test that tests telemetry processing
182191
// within the full BuildRunnerConfig function context
183192
logger.Initialize()
@@ -195,7 +204,7 @@ func TestBuildRunnerConfig_TelemetryProcessing_Integration(t *testing.T) {
195204
require.NoError(t, err)
196205
err = cmd.Flags().Set("otel-sampling-rate", "0.7")
197206
require.NoError(t, err)
198-
cleanup := mockConfig(t, &config.Config{
207+
configProvider, cleanup := createTestConfigProvider(t, &config.Config{
199208
OTEL: config.OpenTelemetryConfig{
200209
Endpoint: "https://config-fallback.example.com",
201210
SamplingRate: 0.2,
@@ -204,7 +213,7 @@ func TestBuildRunnerConfig_TelemetryProcessing_Integration(t *testing.T) {
204213
})
205214
defer cleanup()
206215

207-
configInstance := config.GetConfig()
216+
configInstance := configProvider.GetConfig()
208217
finalEndpoint, finalSamplingRate, finalEnvVars := getTelemetryFromFlags(
209218
cmd,
210219
configInstance,

‎pkg/api/v1/registry.go‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,23 @@ const (
3131

3232
// getRegistryInfo returns the registry type and the source
3333
func getRegistryInfo() (RegistryType, string) {
34-
url, localPath, _, registryType := config.GetRegistryConfig()
35-
36-
switch registryType {
37-
case "url":
38-
return RegistryTypeURL, url
39-
case "file":
40-
return RegistryTypeFile, localPath
41-
default:
42-
// Default built-in registry
43-
return RegistryTypeDefault, ""
34+
return getRegistryInfoWithProvider(config.NewDefaultProvider())
35+
}
36+
37+
// getRegistryInfoWithProvider returns the registry type and the source using the provided config provider
38+
func getRegistryInfoWithProvider(configProvider config.Provider) (RegistryType, string) {
39+
cfg := configProvider.GetConfig()
40+
41+
if cfg.RegistryUrl != "" {
42+
return RegistryTypeURL, cfg.RegistryUrl
4443
}
44+
45+
if cfg.LocalRegistryPath != "" {
46+
return RegistryTypeFile, cfg.LocalRegistryPath
47+
}
48+
49+
// Default built-in registry
50+
return RegistryTypeDefault, ""
4551
}
4652

4753
// getCurrentProvider returns the current registry provider

‎pkg/api/v1/registry_test.go‎

Lines changed: 26 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"strings"
1111
"testing"
1212

13-
"github.com/adrg/xdg"
1413
"github.com/go-chi/chi/v5"
1514
"github.com/stretchr/testify/assert"
1615
"github.com/stretchr/testify/require"
@@ -19,31 +18,31 @@ import (
1918
"github.com/stacklok/toolhive/pkg/logger"
2019
)
2120

22-
func MockConfig(t *testing.T, cfg *config.Config) func() {
21+
func CreateTestConfigProvider(t *testing.T, cfg *config.Config) (config.Provider, func()) {
2322
t.Helper()
2423

2524
// Create a temporary directory for the test
2625
tempDir := t.TempDir()
2726

28-
// TODO: see if there's a way to avoid changing env vars during tests.
29-
// Save original XDG_CONFIG_HOME
30-
originalXDGConfigHome := os.Getenv("XDG_CONFIG_HOME")
31-
t.Setenv("XDG_CONFIG_HOME", tempDir)
32-
xdg.Reload()
33-
3427
// Create the config directory structure
3528
configDir := filepath.Join(tempDir, "toolhive")
3629
err := os.MkdirAll(configDir, 0755)
3730
require.NoError(t, err)
3831

32+
// Set up the config file path
33+
configPath := filepath.Join(configDir, "config.yaml")
34+
35+
// Create a path-based config provider
36+
provider := config.NewPathProvider(configPath)
37+
3938
// Write the config file if one is provided
4039
if cfg != nil {
41-
err = config.UpdateConfig(func(c *config.Config) { *c = *cfg })
40+
err = provider.UpdateConfig(func(c *config.Config) { *c = *cfg })
4241
require.NoError(t, err)
4342
}
4443

45-
return func() {
46-
t.Setenv("XDG_CONFIG_HOME", originalXDGConfigHome)
44+
return provider, func() {
45+
// Cleanup is handled by t.TempDir()
4746
}
4847
}
4948

@@ -58,41 +57,39 @@ func TestRegistryRouter(t *testing.T) {
5857

5958
//nolint:paralleltest // Cannot use t.Parallel() with t.Setenv() in Go 1.24+
6059
func TestGetRegistryInfo(t *testing.T) {
60+
t.Parallel()
6161
logger.Initialize()
6262

63-
// Setup temporary config to avoid modifying user's real config
64-
cleanup := MockConfig(t, nil)
65-
t.Cleanup(cleanup)
66-
6763
tests := []struct {
6864
name string
69-
setupConfig func()
65+
config *config.Config
7066
expectedType RegistryType
7167
expectedSource string
7268
}{
7369
{
7470
name: "default registry",
75-
setupConfig: func() {
76-
_ = config.UnsetRegistry()
71+
config: &config.Config{
72+
RegistryUrl: "",
73+
LocalRegistryPath: "",
7774
},
7875
expectedType: RegistryTypeDefault,
7976
expectedSource: "",
8077
},
8178
{
8279
name: "URL registry",
83-
setupConfig: func() {
84-
_ = config.SetRegistryURL("https://test.com/registry.json", false)
80+
config: &config.Config{
81+
RegistryUrl: "https://test.com/registry.json",
82+
AllowPrivateRegistryIp: false,
83+
LocalRegistryPath: "",
8584
},
8685
expectedType: RegistryTypeURL,
8786
expectedSource: "https://test.com/registry.json",
8887
},
8988
{
9089
name: "file registry",
91-
setupConfig: func() {
92-
_ = config.UnsetRegistry()
93-
_ = config.UpdateConfig(func(c *config.Config) {
94-
c.LocalRegistryPath = "/tmp/test-registry.json"
95-
})
90+
config: &config.Config{
91+
RegistryUrl: "",
92+
LocalRegistryPath: "/tmp/test-registry.json",
9693
},
9794
expectedType: RegistryTypeFile,
9895
expectedSource: "/tmp/test-registry.json",
@@ -101,87 +98,17 @@ func TestGetRegistryInfo(t *testing.T) {
10198

10299
for _, tt := range tests {
103100
t.Run(tt.name, func(t *testing.T) {
104-
if tt.setupConfig != nil {
105-
tt.setupConfig()
106-
}
101+
t.Parallel()
102+
configProvider, cleanup := CreateTestConfigProvider(t, tt.config)
103+
defer cleanup()
107104

108-
registryType, source := getRegistryInfo()
105+
registryType, source := getRegistryInfoWithProvider(configProvider)
109106
assert.Equal(t, tt.expectedType, registryType, "Registry type should match expected")
110107
assert.Equal(t, tt.expectedSource, source, "Registry source should match expected")
111108
})
112109
}
113110
}
114111

115-
//nolint:paralleltest // uses MockConfig (env mutation)
116-
func TestSetRegistryURL_SchemeAndPrivateIPs(t *testing.T) {
117-
logger.Initialize()
118-
119-
// Isolate config (XDG) for each run
120-
cleanup := MockConfig(t, nil)
121-
t.Cleanup(cleanup)
122-
123-
tests := []struct {
124-
name string
125-
url string
126-
allowPrivateIPs bool
127-
wantErr bool
128-
}{
129-
// Scheme enforcement when NOT allowing private IPs
130-
{
131-
name: "reject http (public) when not allowing private IPs",
132-
url: "http://example.com/registry.json",
133-
allowPrivateIPs: false,
134-
wantErr: true,
135-
},
136-
{
137-
name: "accept https (public) when not allowing private IPs",
138-
url: "https://example.com/registry.json",
139-
allowPrivateIPs: false,
140-
wantErr: false,
141-
},
142-
143-
// When allowing private IPs, https is allowed to private hosts
144-
{
145-
name: "accept https to loopback when allowing private IPs",
146-
url: "https://127.0.0.1/registry.json",
147-
allowPrivateIPs: true,
148-
wantErr: false,
149-
},
150-
{
151-
name: "accept http to loopback when allowing private IPs",
152-
url: "http://127.0.0.1/registry.json",
153-
allowPrivateIPs: true,
154-
wantErr: false,
155-
},
156-
}
157-
158-
for _, tt := range tests {
159-
tt := tt
160-
t.Run(tt.name, func(t *testing.T) {
161-
// Start from a clean slate each case
162-
require.NoError(t, config.UnsetRegistry())
163-
164-
err := config.SetRegistryURL(tt.url, tt.allowPrivateIPs)
165-
if tt.wantErr {
166-
require.Error(t, err, "expected error but got nil")
167-
168-
// Verify nothing was persisted
169-
regType, src := getRegistryInfo()
170-
assert.Equal(t, RegistryTypeDefault, regType, "registry should remain default on error")
171-
assert.Equal(t, "", src, "source should be empty on error")
172-
return
173-
}
174-
175-
require.NoError(t, err, "unexpected error from SetRegistryURL")
176-
177-
// Confirm via the same helper used elsewhere
178-
regType, src := getRegistryInfo()
179-
assert.Equal(t, RegistryTypeURL, regType, "should be URL type after successful SetRegistryURL")
180-
assert.Equal(t, tt.url, src, "source should be the URL we set")
181-
})
182-
}
183-
}
184-
185112
func TestRegistryAPI_PutEndpoint(t *testing.T) {
186113
t.Parallel()
187114

0 commit comments

Comments
 (0)