diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 16047c3ab67..02b1319dd79 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -196,9 +196,11 @@ func (s *Service) Upsert(ctx context.Context, settings *models.SSOSettings) erro return err } - settings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets) + // make a copy of current settings for reload operation and apply overrides + reloadSettings := *settings + reloadSettings.Settings = overrideMaps(storedSettings.Settings, settings.Settings, secrets) - go s.reload(social, settings.Provider, *settings) + go s.reload(social, settings.Provider, reloadSettings) return nil } diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index b29e2a509d8..333efa33a04 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -26,6 +26,8 @@ import ( ) func TestService_GetForProvider(t *testing.T) { + t.Parallel() + testCases := []struct { name string setup func(env testEnv) @@ -185,7 +187,13 @@ func TestService_GetForProvider(t *testing.T) { } for _, tc := range testCases { + // create a local copy of "tc" to allow concurrent access within tests to the different items of testCases, + // otherwise it would be like a moving pointer while tests run in parallel + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) if tc.setup != nil { tc.setup(env) @@ -207,6 +215,8 @@ func TestService_GetForProvider(t *testing.T) { } func TestService_GetForProviderWithRedactedSecrets(t *testing.T) { + t.Parallel() + testCases := []struct { name string setup func(env testEnv) @@ -286,7 +296,13 @@ func TestService_GetForProviderWithRedactedSecrets(t *testing.T) { } for _, tc := range testCases { + // create a local copy of "tc" to allow concurrent access within tests to the different items of testCases, + // otherwise it would be like a moving pointer while tests run in parallel + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) if tc.setup != nil { tc.setup(env) @@ -306,6 +322,8 @@ func TestService_GetForProviderWithRedactedSecrets(t *testing.T) { } func TestService_List(t *testing.T) { + t.Parallel() + testCases := []struct { name string setup func(env testEnv) @@ -429,7 +447,13 @@ func TestService_List(t *testing.T) { }, } for _, tc := range testCases { + // create a local copy of "tc" to allow concurrent access within tests to the different items of testCases, + // otherwise it would be like a moving pointer while tests run in parallel + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) if tc.setup != nil { tc.setup(env) @@ -449,6 +473,8 @@ func TestService_List(t *testing.T) { } func TestService_ListWithRedactedSecrets(t *testing.T) { + t.Parallel() + testCases := []struct { name string setup func(env testEnv) @@ -723,7 +749,13 @@ func TestService_ListWithRedactedSecrets(t *testing.T) { }, } for _, tc := range testCases { + // create a local copy of "tc" to allow concurrent access within tests to the different items of testCases, + // otherwise it would be like a moving pointer while tests run in parallel + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) if tc.setup != nil { tc.setup(env) @@ -743,7 +775,11 @@ func TestService_ListWithRedactedSecrets(t *testing.T) { } func TestService_Upsert(t *testing.T) { + t.Parallel() + t.Run("successfully upsert SSO settings", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -762,7 +798,7 @@ func TestService_Upsert(t *testing.T) { reloadable := ssosettingstests.NewMockReloadable(t) reloadable.On("Validate", mock.Anything, settings).Return(nil) reloadable.On("Reload", mock.Anything, mock.MatchedBy(func(settings models.SSOSettings) bool { - wg.Done() + defer wg.Done() return settings.Provider == provider && settings.ID == "someid" && maps.Equal(settings.Settings, map[string]any{ @@ -805,6 +841,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("returns error if provider is not configurable", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.GrafanaComProviderName @@ -826,6 +864,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("returns error if provider was not found in reloadables", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -848,6 +888,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("returns error if validation fails", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -870,6 +912,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("returns error if a fallback strategy is not available for the provider", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) settings := &models.SSOSettings{ @@ -889,6 +933,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("returns error if secrets encryption failed", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.OktaProviderName @@ -912,6 +958,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("should not update the current secret if the secret has not been updated", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -947,6 +995,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("returns error if store failed to upsert settings", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -977,6 +1027,8 @@ func TestService_Upsert(t *testing.T) { }) t.Run("successfully upsert SSO settings if reload fails", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -1005,7 +1057,11 @@ func TestService_Upsert(t *testing.T) { } func TestService_Delete(t *testing.T) { + t.Parallel() + t.Run("successfully delete SSO settings", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) var wg sync.WaitGroup @@ -1042,6 +1098,8 @@ func TestService_Delete(t *testing.T) { }) t.Run("return error if SSO setting was not found for the specified provider", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -1051,10 +1109,13 @@ func TestService_Delete(t *testing.T) { err := env.service.Delete(context.Background(), provider) require.Error(t, err) + require.ErrorIs(t, err, ssosettings.ErrNotFound) }) t.Run("should not delete the SSO settings if the provider is not configurable", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) env.cfg.SSOSettingsConfigurableProviders = map[string]bool{social.AzureADProviderName: true} @@ -1066,6 +1127,8 @@ func TestService_Delete(t *testing.T) { }) t.Run("return error when store fails to delete the SSO settings for the specified provider", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -1077,6 +1140,8 @@ func TestService_Delete(t *testing.T) { }) t.Run("return successfully when the deletion was successful but reloading the settings fail", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := social.AzureADProviderName @@ -1094,7 +1159,11 @@ func TestService_Delete(t *testing.T) { } func TestService_DoReload(t *testing.T) { + t.Parallel() + t.Run("successfully reload settings", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) settingsList := []*models.SSOSettings{ @@ -1133,6 +1202,8 @@ func TestService_DoReload(t *testing.T) { }) t.Run("failed fetching the SSO settings", func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) provider := "github" @@ -1147,6 +1218,8 @@ func TestService_DoReload(t *testing.T) { } func TestService_decryptSecrets(t *testing.T) { + t.Parallel() + testCases := []struct { name string setup func(env testEnv) @@ -1219,7 +1292,13 @@ func TestService_decryptSecrets(t *testing.T) { } for _, tc := range testCases { + // create a local copy of "tc" to allow concurrent access within tests to the different items of testCases, + // otherwise it would be like a moving pointer while tests run in parallel + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + env := setupTestEnv(t) if tc.setup != nil { @@ -1242,6 +1321,8 @@ func TestService_decryptSecrets(t *testing.T) { } func setupTestEnv(t *testing.T) testEnv { + t.Helper() + store := ssosettingstests.NewFakeStore() fallbackStrategy := ssosettingstests.NewFakeFallbackStrategy() secrets := secretsFakes.NewMockService(t)