From 68588dc7df8e0255116ec208c6a82dd95a1e3c8e Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Tue, 24 Aug 2021 17:23:51 +0200 Subject: [PATCH] util: fix unit-test for GetClusterMappingInfo() Unit-testing often fails due to a race condition while writing the clusterMappingConfigFile from multiple go-routines at the same time. Failures from `make containerized-test` look like this: === CONT TestGetClusterMappingInfo/site2-storage_cluster-id_mapping cluster_mapping_test.go:153: GetClusterMappingInfo() = , expected data &[{map[site1-storage:site2-storage] [map[1:3]] [map[11:5]]} {map[site3-storage:site2-storage] [map[8:3]] [map[10:5]]}] === CONT TestGetClusterMappingInfo/site3-storage_cluster-id_mapping cluster_mapping_test.go:153: GetClusterMappingInfo() = , expected data &[{map[site3-storage:site2-storage] [map[8:3]] [map[10:5]]}] --- FAIL: TestGetClusterMappingInfo (0.01s) --- PASS: TestGetClusterMappingInfo/mapping_file_not_found (0.00s) --- PASS: TestGetClusterMappingInfo/mapping_file_found_with_empty_data (0.00s) --- PASS: TestGetClusterMappingInfo/cluster-id_mapping_not_found (0.00s) --- FAIL: TestGetClusterMappingInfo/site2-storage_cluster-id_mapping (0.00s) --- FAIL: TestGetClusterMappingInfo/site3-storage_cluster-id_mapping (0.00s) --- PASS: TestGetClusterMappingInfo/site1-storage_cluster-id_mapping (0.00s) By splitting the public GetClusterMappingInfo() function into an internal getClusterMappingInfo() that takes a filename, unit-testing can use different files for each go-routine, and testing becomes more predictable. Signed-off-by: Niels de Vos --- internal/util/cluster_mapping.go | 18 ++++++++++++------ internal/util/cluster_mapping_test.go | 12 ++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/internal/util/cluster_mapping.go b/internal/util/cluster_mapping.go index 51af34f9a..173f5126e 100644 --- a/internal/util/cluster_mapping.go +++ b/internal/util/cluster_mapping.go @@ -65,9 +65,9 @@ type ClusterMappingInfo struct { // ... // }] -func readClusterMappingInfo() (*[]ClusterMappingInfo, error) { +func readClusterMappingInfo(filename string) (*[]ClusterMappingInfo, error) { var info []ClusterMappingInfo - content, err := ioutil.ReadFile(clusterMappingConfigFile) + content, err := ioutil.ReadFile(filename) // #nosec:G304, file inclusion via variable. if err != nil { err = fmt.Errorf("error fetching clusterID mapping %w", err) @@ -83,11 +83,11 @@ func readClusterMappingInfo() (*[]ClusterMappingInfo, error) { return &info, nil } -// GetClusterMappingInfo returns corresponding cluster details like clusterID's -// poolID,fscID lists read from configfile. -func GetClusterMappingInfo(clusterID string) (*[]ClusterMappingInfo, error) { +// getClusterMappingInfo returns corresponding cluster details like clusterID's +// poolID,fscID lists read from 'filename'. +func getClusterMappingInfo(clusterID, filename string) (*[]ClusterMappingInfo, error) { var mappingInfo []ClusterMappingInfo - info, err := readClusterMappingInfo() + info, err := readClusterMappingInfo(filename) if err != nil { // discard not found error as this file is expected to be created by // the admin in case of failover. @@ -114,3 +114,9 @@ func GetClusterMappingInfo(clusterID string) (*[]ClusterMappingInfo, error) { return &mappingInfo, nil } + +// GetClusterMappingInfo returns corresponding cluster details like clusterID's +// poolID,fscID lists read from configfile. +func GetClusterMappingInfo(clusterID string) (*[]ClusterMappingInfo, error) { + return getClusterMappingInfo(clusterID, clusterMappingConfigFile) +} diff --git a/internal/util/cluster_mapping_test.go b/internal/util/cluster_mapping_test.go index 92aa2da5c..2a85ed9ef 100644 --- a/internal/util/cluster_mapping_test.go +++ b/internal/util/cluster_mapping_test.go @@ -138,19 +138,19 @@ func TestGetClusterMappingInfo(t *testing.T) { currentTT := tt t.Run(currentTT.name, func(t *testing.T) { t.Parallel() - clusterMappingConfigFile = fmt.Sprintf("%s/mapping-%d.json", mappingBasePath, currentI) + mappingConfigFile := fmt.Sprintf("%s/mapping-%d.json", mappingBasePath, currentI) if len(currentTT.mappingFilecontent) != 0 { - err = ioutil.WriteFile(clusterMappingConfigFile, currentTT.mappingFilecontent, 0o600) + err = ioutil.WriteFile(mappingConfigFile, currentTT.mappingFilecontent, 0o600) if err != nil { - t.Errorf("GetClusterMappingInfo() error = %v", err) + t.Errorf("failed to write to %q, error = %v", mappingConfigFile, err) } } - data, mErr := GetClusterMappingInfo(currentTT.clusterID) + data, mErr := getClusterMappingInfo(currentTT.clusterID, mappingConfigFile) if (mErr != nil) != currentTT.expectErr { - t.Errorf("GetClusterMappingInfo() error = %v, expected Error %v", mErr, currentTT.expectErr) + t.Errorf("getClusterMappingInfo() error = %v, expected Error %v", mErr, currentTT.expectErr) } if !reflect.DeepEqual(data, currentTT.expectedData) { - t.Errorf("GetClusterMappingInfo() = %v, expected data %v", data, currentTT.expectedData) + t.Errorf("getClusterMappingInfo() = %v, expected data %v", data, currentTT.expectedData) } }) }