From 3326df42bb91bf684f65b0a438ed5502a98bfd58 Mon Sep 17 00:00:00 2001 From: Leonardo Zamariola Date: Thu, 30 Jun 2022 13:20:16 -0300 Subject: [PATCH] Removing global state modification on unit tests (fix #10033 #10034) (#10935) * Removing global state modification on unit tests (fix #10033 #10034) The config.DefaultRemoteReadConfig and config.DefaultRemoteWriteConfig instances hold global state. Unit tests were changing their url.URL reference globally causing false positives when tests were ran through package. Two helper functions were created to copy those global values instead of changing them in place to fix null point when running unit tests by method instead of by package. Signed-off-by: Leonardo Zamariola * Fixing pull request suggestions Copying by value from default config Signed-off-by: Leonardo Zamariola --- storage/remote/queue_manager_test.go | 11 ++----- storage/remote/storage_test.go | 46 ++++++++++++++++++---------- storage/remote/write_test.go | 15 ++------- 3 files changed, 34 insertions(+), 38 deletions(-) diff --git a/storage/remote/queue_manager_test.go b/storage/remote/queue_manager_test.go index cb5cee9a4..aec783c4d 100644 --- a/storage/remote/queue_manager_test.go +++ b/storage/remote/queue_manager_test.go @@ -17,7 +17,6 @@ import ( "context" "fmt" "math" - "net/url" "os" "runtime/pprof" "sort" @@ -32,7 +31,6 @@ import ( "github.com/golang/snappy" "github.com/prometheus/client_golang/prometheus" client_testutil "github.com/prometheus/client_golang/prometheus/testutil" - common_config "github.com/prometheus/common/config" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" "go.uber.org/atomic" @@ -84,20 +82,15 @@ func TestSampleDelivery(t *testing.T) { queueConfig.BatchSendDeadline = model.Duration(100 * time.Millisecond) queueConfig.MaxShards = 1 - writeConfig := config.DefaultRemoteWriteConfig // We need to set URL's so that metric creation doesn't panic. - writeConfig.URL = &common_config.URL{ - URL: &url.URL{ - Host: "http://test-storage.com", - }, - } + writeConfig := baseRemoteWriteConfig("http://test-storage.com") writeConfig.QueueConfig = queueConfig writeConfig.SendExemplars = true conf := &config.Config{ GlobalConfig: config.DefaultGlobalConfig, RemoteWriteConfigs: []*config.RemoteWriteConfig{ - &writeConfig, + writeConfig, }, } diff --git a/storage/remote/storage_test.go b/storage/remote/storage_test.go index da2b689c0..5723a6d0c 100644 --- a/storage/remote/storage_test.go +++ b/storage/remote/storage_test.go @@ -31,21 +31,11 @@ func TestStorageLifecycle(t *testing.T) { conf := &config.Config{ GlobalConfig: config.DefaultGlobalConfig, RemoteWriteConfigs: []*config.RemoteWriteConfig{ - &config.DefaultRemoteWriteConfig, + // We need to set URL's so that metric creation doesn't panic. + baseRemoteWriteConfig("http://test-storage.com"), }, RemoteReadConfigs: []*config.RemoteReadConfig{ - &config.DefaultRemoteReadConfig, - }, - } - // We need to set URL's so that metric creation doesn't panic. - conf.RemoteWriteConfigs[0].URL = &common_config.URL{ - URL: &url.URL{ - Host: "http://test-storage.com", - }, - } - conf.RemoteReadConfigs[0].URL = &common_config.URL{ - URL: &url.URL{ - Host: "http://test-storage.com", + baseRemoteReadConfig("http://test-storage.com"), }, } @@ -73,7 +63,7 @@ func TestUpdateRemoteReadConfigs(t *testing.T) { require.Equal(t, 0, len(s.queryables)) conf.RemoteReadConfigs = []*config.RemoteReadConfig{ - &config.DefaultRemoteReadConfig, + baseRemoteReadConfig("http://test-storage.com"), } require.NoError(t, s.ApplyConfig(conf)) require.Equal(t, 1, len(s.queryables)) @@ -96,7 +86,7 @@ func TestFilterExternalLabels(t *testing.T) { require.Equal(t, 0, len(s.queryables)) conf.RemoteReadConfigs = []*config.RemoteReadConfig{ - &config.DefaultRemoteReadConfig, + baseRemoteReadConfig("http://test-storage.com"), } require.NoError(t, s.ApplyConfig(conf)) @@ -121,7 +111,7 @@ func TestIgnoreExternalLabels(t *testing.T) { require.Equal(t, 0, len(s.queryables)) conf.RemoteReadConfigs = []*config.RemoteReadConfig{ - &config.DefaultRemoteReadConfig, + baseRemoteReadConfig("http://test-storage.com"), } conf.RemoteReadConfigs[0].FilterExternalLabels = false @@ -133,3 +123,27 @@ func TestIgnoreExternalLabels(t *testing.T) { err := s.Close() require.NoError(t, err) } + +// baseRemoteWriteConfig copy values from global Default Write config +// to avoid change global state and cross impact test execution +func baseRemoteWriteConfig(host string) *config.RemoteWriteConfig { + cfg := config.DefaultRemoteWriteConfig + cfg.URL = &common_config.URL{ + URL: &url.URL{ + Host: host, + }, + } + return &cfg +} + +// baseRemoteReadConfig copy values from global Default Read config +// to avoid change global state and cross impact test execution +func baseRemoteReadConfig(host string) *config.RemoteReadConfig { + cfg := config.DefaultRemoteReadConfig + cfg.URL = &common_config.URL{ + URL: &url.URL{ + Host: host, + }, + } + return &cfg +} diff --git a/storage/remote/write_test.go b/storage/remote/write_test.go index d92b65852..3210f9018 100644 --- a/storage/remote/write_test.go +++ b/storage/remote/write_test.go @@ -202,7 +202,7 @@ func TestWriteStorageLifecycle(t *testing.T) { conf := &config.Config{ GlobalConfig: config.DefaultGlobalConfig, RemoteWriteConfigs: []*config.RemoteWriteConfig{ - &config.DefaultRemoteWriteConfig, + baseRemoteWriteConfig("http://test-storage.com"), }, } require.NoError(t, s.ApplyConfig(conf)) @@ -249,18 +249,7 @@ func TestWriteStorageApplyConfigsIdempotent(t *testing.T) { conf := &config.Config{ GlobalConfig: config.GlobalConfig{}, RemoteWriteConfigs: []*config.RemoteWriteConfig{ - { - RemoteTimeout: config.DefaultRemoteWriteConfig.RemoteTimeout, - QueueConfig: config.DefaultRemoteWriteConfig.QueueConfig, - MetadataConfig: config.DefaultRemoteWriteConfig.MetadataConfig, - HTTPClientConfig: config.DefaultRemoteWriteConfig.HTTPClientConfig, - // We need to set URL's so that metric creation doesn't panic. - URL: &common_config.URL{ - URL: &url.URL{ - Host: "http://test-storage.com", - }, - }, - }, + baseRemoteWriteConfig("http://test-storage.com"), }, } hash, err := toHash(conf.RemoteWriteConfigs[0])