From 29b7ae1c345209c14f49e691dee2052b766ea75d Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Mon, 9 Oct 2023 18:05:05 +0200 Subject: [PATCH 01/10] SNOW-856228 easy logging parser --- client_configuration.go | 84 ++++++++++++++++++ client_configuration_test.go | 164 +++++++++++++++++++++++++++++++++++ go.mod | 5 +- 3 files changed, 252 insertions(+), 1 deletion(-) create mode 100644 client_configuration.go create mode 100644 client_configuration_test.go diff --git a/client_configuration.go b/client_configuration.go new file mode 100644 index 000000000..2858c738b --- /dev/null +++ b/client_configuration.go @@ -0,0 +1,84 @@ +// Copyright (c) 2023 Snowflake Computing Inc. All rights reserved. + +package gosnowflake + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "strings" +) + +const ( + Off string = "OFF" + Error string = "ERROR" + Warn string = "WARN" + Info string = "INFO" + Debug string = "DEBUG" + Trace string = "TRACE" +) + +type ClientConfig struct { + Common *ClientConfigCommonProps `json:common` +} + +type ClientConfigCommonProps struct { + LogLevel *string `json:"log_level"` + LogPath *string `json:"log_path"` +} + +func parseClientConfiguration(filePath string) (*ClientConfig, error) { + if filePath == "" { + return nil, nil + } + fileContents, readError := os.ReadFile(filePath) + if readError != nil { + return nil, parsingClientConfigError(readError) + } + var clientConfig ClientConfig + parseError := json.Unmarshal(fileContents, &clientConfig) + if parseError != nil { + return nil, parsingClientConfigError(parseError) + } + validateError := validateClientConfiguration(&clientConfig) + if validateError != nil { + return nil, parsingClientConfigError(validateError) + } + return &clientConfig, nil +} + +func parsingClientConfigError(err error) error { + return fmt.Errorf("parsing client config failed: %w", err) +} + +func validateClientConfiguration(clientConfig *ClientConfig) error { + if clientConfig == nil { + return errors.New("client config not found") + } + if clientConfig.Common == nil { + return errors.New("common section in client config not found") + } + return validateLogLevel(clientConfig) +} + +func validateLogLevel(clientConfig *ClientConfig) error { + var logLevel = clientConfig.Common.LogLevel + if logLevel != nil && *logLevel != "" { + _, error := toLogLevel(*logLevel) + if error != nil { + return error + } + } + return nil +} + +func toLogLevel(logLevelString string) (*string, error) { + var logLevel = strings.ToUpper(logLevelString) + switch logLevel { + case Off, Error, Warn, Info, Debug, Trace: + return &logLevel, nil + default: + return nil, errors.New("unknown log level: " + logLevelString) + } +} diff --git a/client_configuration_test.go b/client_configuration_test.go new file mode 100644 index 000000000..29c92798e --- /dev/null +++ b/client_configuration_test.go @@ -0,0 +1,164 @@ +// Copyright (c) 2023 Snowflake Computing Inc. All rights reserved. + +package gosnowflake + +import ( + "fmt" + "github.com/stretchr/testify/assert" + "os" + "path" + "strings" + "testing" +) + +type PositiveTestCase struct { + Name string + FileName string + FileContents string + ExpectedLogLevel *string + ExpectedLogPath *string +} + +type NegativeTestCase struct { + Name string + FileName string + FileContents string + ExpectedErrorMessageToContain string +} + +func TestThatParsesConfiguration(t *testing.T) { + // given + dir := CreateTempDirectory(t, "conf_parse_tests_") + testCases := []PositiveTestCase{ + { + Name: "TestWithLogLevelUpperCase", + FileName: "config.json", + FileContents: `{ + "common": { + "log_level" : "INFO", + "log_path" : "/some-path/some-directory" + } + }`, + ExpectedLogLevel: toStringPointer("INFO"), + ExpectedLogPath: toStringPointer("/some-path/some-directory"), + }, + { + Name: "TestWithLogLevelLowerCase", + FileName: "config.json", + FileContents: `{ + "common": { + "log_level" : "info", + "log_path" : "/some-path/some-directory" + } + }`, + ExpectedLogLevel: toStringPointer("info"), + ExpectedLogPath: toStringPointer("/some-path/some-directory"), + }, + { + Name: "TestWithMissingValues", + FileName: "config.json", + FileContents: `{ + "common": {} + }`, + ExpectedLogLevel: nil, + ExpectedLogPath: nil, + }, + } + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + fileName := CreateFile(t, testCase.FileName, testCase.FileContents, dir) + + // when + config, err := parseClientConfiguration(fileName) + + // then + assert := assert.New(t) + assert.Equal(nil, err, "Error should be nil") + assert.Equal(testCase.ExpectedLogLevel, config.Common.LogLevel, "Log level should be as expected") + assert.Equal(testCase.ExpectedLogPath, config.Common.LogPath, "Log path should be as expected") + }) + } +} + +func TestThatFailsToParse(t *testing.T) { + // given + dir := CreateTempDirectory(t, "conf_negative_parse_tests_") + testCases := []NegativeTestCase{ + { + Name: "TestWithWrongLogLevel", + FileName: "config.json", + FileContents: `{ + "common": { + "log_level" : "something weird", + "log_path" : "/some-path/some-directory" + } + }`, + ExpectedErrorMessageToContain: "unknown log level", + }, + { + Name: "TestWithWrongTypeOfLogLevel", + FileName: "config.json", + FileContents: `{ + "common": { + "log_level" : 15, + "log_path" : "/some-path/some-directory" + } + }`, + ExpectedErrorMessageToContain: "ClientConfigCommonProps.Common.log_level", + }, + { + Name: "TestWithWrongTypeOfLogPath", + FileName: "config.json", + FileContents: `{ + "common": { + "log_level" : "INFO", + "log_path" : true + } + }`, + ExpectedErrorMessageToContain: "ClientConfigCommonProps.Common.log_path", + }, + } + for _, testCase := range testCases { + t.Run(testCase.Name, func(t *testing.T) { + fileName := CreateFile(t, testCase.FileName, testCase.FileContents, dir) + + // when + _, err := parseClientConfiguration(fileName) + + // then + assert := assert.New(t) + assert.Equal(err != nil, true, "Error should not be nil") + errMessage := fmt.Sprint(err) + expectedPrefix := "parsing client config failed" + assert.Equal(strings.HasPrefix(errMessage, expectedPrefix), true, + fmt.Sprintf("Error message: \"%s\" should start with prefix: \"%s\"", errMessage, expectedPrefix)) + assert.Equal(strings.Contains(errMessage, testCase.ExpectedErrorMessageToContain), true, + fmt.Sprintf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, testCase.ExpectedErrorMessageToContain)) + }) + } +} + +func toStringPointer(value string) *string { + var copyOfValue = value + return ©OfValue +} + +func CreateFile(t *testing.T, fileName string, fileContents string, directory string) string { + fullFileName := path.Join(directory, fileName) + writeErr := os.WriteFile(fullFileName, []byte(fileContents), 0644) + if writeErr != nil { + t.Error("Could not create file") + } + return fullFileName +} + +func CreateTempDirectory(t *testing.T, dirPattern string) string { + dir, dirErr := os.MkdirTemp(os.TempDir(), dirPattern) + if dirErr != nil { + t.Error("Failed to create test directory") + } + t.Cleanup(func() { + os.RemoveAll(dir) + }) + return dir +} diff --git a/go.mod b/go.mod index d78792d8f..718e20311 100644 --- a/go.mod +++ b/go.mod @@ -16,6 +16,7 @@ require ( github.com/gabriel-vasile/mimetype v1.4.2 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 github.com/sirupsen/logrus v1.9.0 + github.com/stretchr/testify v1.8.1 golang.org/x/crypto v0.7.0 ) @@ -34,6 +35,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.9.25 // indirect github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.14.0 // indirect github.com/danieljoos/wincred v1.1.2 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect github.com/dvsekhvalnov/jose2go v1.5.0 // indirect github.com/goccy/go-json v0.10.0 // indirect github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect @@ -50,7 +52,7 @@ require ( github.com/minio/c2goasm v0.0.0-20190812172519-36a3d3bbc4f3 // indirect github.com/mtibben/percent v0.2.1 // indirect github.com/pierrec/lz4/v4 v4.1.17 // indirect - github.com/stretchr/testify v1.8.1 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect github.com/zeebo/xxh3 v1.0.2 // indirect golang.org/x/exp v0.0.0-20230206171751-46f607a40771 // indirect golang.org/x/mod v0.8.0 // indirect @@ -61,4 +63,5 @@ require ( golang.org/x/text v0.8.0 // indirect golang.org/x/tools v0.6.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) From c37672513257ae4c1a624e94e023af3ffc5c3feb Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Wed, 11 Oct 2023 12:24:39 +0200 Subject: [PATCH 02/10] fix linter comments --- client_configuration.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/client_configuration.go b/client_configuration.go index 2858c738b..bdeeb4a82 100644 --- a/client_configuration.go +++ b/client_configuration.go @@ -10,19 +10,22 @@ import ( "strings" ) +// log levels for easy logging const ( - Off string = "OFF" - Error string = "ERROR" - Warn string = "WARN" - Info string = "INFO" - Debug string = "DEBUG" - Trace string = "TRACE" + Off string = "OFF" // log level for logging switched off + Error string = "ERROR" // error log level + Warn string = "WARN" // warn log level + Info string = "INFO" // info log level + Debug string = "DEBUG" // debug log level + Trace string = "TRACE" // trace log level ) +// ClientConfig properties root type ClientConfig struct { - Common *ClientConfigCommonProps `json:common` + Common *ClientConfigCommonProps `json:"common"` } +// ClientConfigCommonProps properties from "common" section type ClientConfigCommonProps struct { LogLevel *string `json:"log_level"` LogPath *string `json:"log_path"` From 04082c1d07d62dde756dfd16e7033059630f455a Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Wed, 11 Oct 2023 12:58:43 +0200 Subject: [PATCH 03/10] fix error message tests --- client_configuration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client_configuration_test.go b/client_configuration_test.go index 29c92798e..0f53a3db0 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -104,7 +104,7 @@ func TestThatFailsToParse(t *testing.T) { "log_path" : "/some-path/some-directory" } }`, - ExpectedErrorMessageToContain: "ClientConfigCommonProps.Common.log_level", + ExpectedErrorMessageToContain: "ClientConfigCommonProps.common.log_level", }, { Name: "TestWithWrongTypeOfLogPath", @@ -115,7 +115,7 @@ func TestThatFailsToParse(t *testing.T) { "log_path" : true } }`, - ExpectedErrorMessageToContain: "ClientConfigCommonProps.Common.log_path", + ExpectedErrorMessageToContain: "ClientConfigCommonProps.common.log_path", }, } for _, testCase := range testCases { From d08797478f162bb2f52d90fc8baa3cce0e66ed9d Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Wed, 11 Oct 2023 15:29:27 +0200 Subject: [PATCH 04/10] fix review comments --- client_configuration.go | 20 +++---- client_configuration_test.go | 108 +++++++++++++++++------------------ 2 files changed, 64 insertions(+), 64 deletions(-) diff --git a/client_configuration.go b/client_configuration.go index bdeeb4a82..03c70b6c0 100644 --- a/client_configuration.go +++ b/client_configuration.go @@ -20,7 +20,7 @@ const ( Trace string = "TRACE" // trace log level ) -// ClientConfig properties root +// ClientConfig config root type ClientConfig struct { Common *ClientConfigCommonProps `json:"common"` } @@ -35,18 +35,18 @@ func parseClientConfiguration(filePath string) (*ClientConfig, error) { if filePath == "" { return nil, nil } - fileContents, readError := os.ReadFile(filePath) - if readError != nil { - return nil, parsingClientConfigError(readError) + fileContents, readErr := os.ReadFile(filePath) + if readErr != nil { + return nil, parsingClientConfigError(readErr) } var clientConfig ClientConfig - parseError := json.Unmarshal(fileContents, &clientConfig) - if parseError != nil { - return nil, parsingClientConfigError(parseError) + parseErr := json.Unmarshal(fileContents, &clientConfig) + if parseErr != nil { + return nil, parsingClientConfigError(parseErr) } - validateError := validateClientConfiguration(&clientConfig) - if validateError != nil { - return nil, parsingClientConfigError(validateError) + validateErr := validateClientConfiguration(&clientConfig) + if validateErr != nil { + return nil, parsingClientConfigError(validateErr) } return &clientConfig, nil } diff --git a/client_configuration_test.go b/client_configuration_test.go index 0f53a3db0..d8216871e 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -11,28 +11,18 @@ import ( "testing" ) -type PositiveTestCase struct { - Name string - FileName string - FileContents string - ExpectedLogLevel *string - ExpectedLogPath *string -} - -type NegativeTestCase struct { - Name string - FileName string - FileContents string - ExpectedErrorMessageToContain string -} - func TestThatParsesConfiguration(t *testing.T) { - // given - dir := CreateTempDirectory(t, "conf_parse_tests_") - testCases := []PositiveTestCase{ + dir := t.TempDir() + testCases := []struct { + Name string + FileName string + FileContents string + ExpectedLogLevel *string + ExpectedLogPath *string + }{ { Name: "TestWithLogLevelUpperCase", - FileName: "config.json", + FileName: "config_1.json", FileContents: `{ "common": { "log_level" : "INFO", @@ -44,7 +34,7 @@ func TestThatParsesConfiguration(t *testing.T) { }, { Name: "TestWithLogLevelLowerCase", - FileName: "config.json", + FileName: "config_2.json", FileContents: `{ "common": { "log_level" : "info", @@ -56,7 +46,7 @@ func TestThatParsesConfiguration(t *testing.T) { }, { Name: "TestWithMissingValues", - FileName: "config.json", + FileName: "config_3.json", FileContents: `{ "common": {} }`, @@ -64,29 +54,52 @@ func TestThatParsesConfiguration(t *testing.T) { ExpectedLogPath: nil, }, } - for _, testCase := range testCases { - t.Run(testCase.Name, func(t *testing.T) { - fileName := CreateFile(t, testCase.FileName, testCase.FileContents, dir) + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + fileName := CreateFile(t, tc.FileName, tc.FileContents, dir) - // when config, err := parseClientConfiguration(fileName) - // then assert := assert.New(t) assert.Equal(nil, err, "Error should be nil") - assert.Equal(testCase.ExpectedLogLevel, config.Common.LogLevel, "Log level should be as expected") - assert.Equal(testCase.ExpectedLogPath, config.Common.LogPath, "Log path should be as expected") + assert.Equal(tc.ExpectedLogLevel, config.Common.LogLevel, "Log level should be as expected") + assert.Equal(tc.ExpectedLogPath, config.Common.LogPath, "Log path should be as expected") }) } } -func TestThatFailsToParse(t *testing.T) { - // given - dir := CreateTempDirectory(t, "conf_negative_parse_tests_") - testCases := []NegativeTestCase{ +func TestParseAllLogLevels(t *testing.T) { + dir := t.TempDir() + for _, logLevel := range []string{"OFF", "ERROR", "WARN", "INFO", "DEBUG", "TRACE"} { + t.Run(logLevel, func(t *testing.T) { + fileContents := fmt.Sprintf(`{ + "common": { + "log_level" : "%s", + "log_path" : "/some-path/some-directory" + } + }`, logLevel) + fileName := CreateFile(t, fmt.Sprintf("config_%s.json", logLevel), fileContents, dir) + + config, err := parseClientConfiguration(fileName) + + assert := assert.New(t) + assert.Equal(nil, err, "Error should be nil") + assert.Equal(logLevel, *config.Common.LogLevel, "Log level should be as expected") + }) + } +} + +func TestParseConfigurationFails(t *testing.T) { + dir := t.TempDir() + testCases := []struct { + Name string + FileName string + FileContents string + ExpectedErrorMessageToContain string + }{ { Name: "TestWithWrongLogLevel", - FileName: "config.json", + FileName: "config_1.json", FileContents: `{ "common": { "log_level" : "something weird", @@ -97,7 +110,7 @@ func TestThatFailsToParse(t *testing.T) { }, { Name: "TestWithWrongTypeOfLogLevel", - FileName: "config.json", + FileName: "config_2.json", FileContents: `{ "common": { "log_level" : 15, @@ -108,7 +121,7 @@ func TestThatFailsToParse(t *testing.T) { }, { Name: "TestWithWrongTypeOfLogPath", - FileName: "config.json", + FileName: "config_3.json", FileContents: `{ "common": { "log_level" : "INFO", @@ -118,22 +131,20 @@ func TestThatFailsToParse(t *testing.T) { ExpectedErrorMessageToContain: "ClientConfigCommonProps.common.log_path", }, } - for _, testCase := range testCases { - t.Run(testCase.Name, func(t *testing.T) { - fileName := CreateFile(t, testCase.FileName, testCase.FileContents, dir) + for _, tc := range testCases { + t.Run(tc.Name, func(t *testing.T) { + fileName := CreateFile(t, tc.FileName, tc.FileContents, dir) - // when _, err := parseClientConfiguration(fileName) - // then assert := assert.New(t) assert.Equal(err != nil, true, "Error should not be nil") errMessage := fmt.Sprint(err) expectedPrefix := "parsing client config failed" assert.Equal(strings.HasPrefix(errMessage, expectedPrefix), true, fmt.Sprintf("Error message: \"%s\" should start with prefix: \"%s\"", errMessage, expectedPrefix)) - assert.Equal(strings.Contains(errMessage, testCase.ExpectedErrorMessageToContain), true, - fmt.Sprintf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, testCase.ExpectedErrorMessageToContain)) + assert.Equal(strings.Contains(errMessage, tc.ExpectedErrorMessageToContain), true, + fmt.Sprintf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, tc.ExpectedErrorMessageToContain)) }) } } @@ -147,18 +158,7 @@ func CreateFile(t *testing.T, fileName string, fileContents string, directory st fullFileName := path.Join(directory, fileName) writeErr := os.WriteFile(fullFileName, []byte(fileContents), 0644) if writeErr != nil { - t.Error("Could not create file") + t.Fatal("Could not create file") } return fullFileName } - -func CreateTempDirectory(t *testing.T, dirPattern string) string { - dir, dirErr := os.MkdirTemp(os.TempDir(), dirPattern) - if dirErr != nil { - t.Error("Failed to create test directory") - } - t.Cleanup(func() { - os.RemoveAll(dir) - }) - return dir -} From 39e613f800cc8360454a4879614d1249ab65e0d9 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Wed, 11 Oct 2023 18:09:10 +0200 Subject: [PATCH 05/10] test for missing common --- client_configuration_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/client_configuration_test.go b/client_configuration_test.go index d8216871e..860260ea5 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -130,6 +130,12 @@ func TestParseConfigurationFails(t *testing.T) { }`, ExpectedErrorMessageToContain: "ClientConfigCommonProps.common.log_path", }, + { + Name: "TestWithoutCommon", + FileName: "config_4.json", + FileContents: "{}", + ExpectedErrorMessageToContain: "common section in client config not found", + }, } for _, tc := range testCases { t.Run(tc.Name, func(t *testing.T) { From a899c7876dc9fedc66d45e1677788ce9e70d12c1 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Thu, 12 Oct 2023 10:45:54 +0200 Subject: [PATCH 06/10] rename test --- client_configuration_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_configuration_test.go b/client_configuration_test.go index 860260ea5..85fcba7b0 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -11,7 +11,7 @@ import ( "testing" ) -func TestThatParsesConfiguration(t *testing.T) { +func TestParseConfiguration(t *testing.T) { dir := t.TempDir() testCases := []struct { Name string From 1f5d0d559362ac28914e1ce68fff1b4268acc05c Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Thu, 12 Oct 2023 17:51:26 +0200 Subject: [PATCH 07/10] make logLevel and logPath strings, not pointers --- client_configuration.go | 18 +++++++++--------- client_configuration_test.go | 23 +++++++++-------------- 2 files changed, 18 insertions(+), 23 deletions(-) diff --git a/client_configuration.go b/client_configuration.go index 03c70b6c0..ea6a637b2 100644 --- a/client_configuration.go +++ b/client_configuration.go @@ -27,8 +27,8 @@ type ClientConfig struct { // ClientConfigCommonProps properties from "common" section type ClientConfigCommonProps struct { - LogLevel *string `json:"log_level"` - LogPath *string `json:"log_path"` + LogLevel string `json:"log_level,omitempty"` + LogPath string `json:"log_path,omitempty"` } func parseClientConfiguration(filePath string) (*ClientConfig, error) { @@ -62,13 +62,13 @@ func validateClientConfiguration(clientConfig *ClientConfig) error { if clientConfig.Common == nil { return errors.New("common section in client config not found") } - return validateLogLevel(clientConfig) + return validateLogLevel(*clientConfig) } -func validateLogLevel(clientConfig *ClientConfig) error { +func validateLogLevel(clientConfig ClientConfig) error { var logLevel = clientConfig.Common.LogLevel - if logLevel != nil && *logLevel != "" { - _, error := toLogLevel(*logLevel) + if logLevel != "" { + _, error := toLogLevel(logLevel) if error != nil { return error } @@ -76,12 +76,12 @@ func validateLogLevel(clientConfig *ClientConfig) error { return nil } -func toLogLevel(logLevelString string) (*string, error) { +func toLogLevel(logLevelString string) (string, error) { var logLevel = strings.ToUpper(logLevelString) switch logLevel { case Off, Error, Warn, Info, Debug, Trace: - return &logLevel, nil + return logLevel, nil default: - return nil, errors.New("unknown log level: " + logLevelString) + return "", errors.New("unknown log level: " + logLevelString) } } diff --git a/client_configuration_test.go b/client_configuration_test.go index 85fcba7b0..bf8004f29 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -17,8 +17,8 @@ func TestParseConfiguration(t *testing.T) { Name string FileName string FileContents string - ExpectedLogLevel *string - ExpectedLogPath *string + ExpectedLogLevel string + ExpectedLogPath string }{ { Name: "TestWithLogLevelUpperCase", @@ -29,8 +29,8 @@ func TestParseConfiguration(t *testing.T) { "log_path" : "/some-path/some-directory" } }`, - ExpectedLogLevel: toStringPointer("INFO"), - ExpectedLogPath: toStringPointer("/some-path/some-directory"), + ExpectedLogLevel: "INFO", + ExpectedLogPath: "/some-path/some-directory", }, { Name: "TestWithLogLevelLowerCase", @@ -41,8 +41,8 @@ func TestParseConfiguration(t *testing.T) { "log_path" : "/some-path/some-directory" } }`, - ExpectedLogLevel: toStringPointer("info"), - ExpectedLogPath: toStringPointer("/some-path/some-directory"), + ExpectedLogLevel: "info", + ExpectedLogPath: "/some-path/some-directory", }, { Name: "TestWithMissingValues", @@ -50,8 +50,8 @@ func TestParseConfiguration(t *testing.T) { FileContents: `{ "common": {} }`, - ExpectedLogLevel: nil, - ExpectedLogPath: nil, + ExpectedLogLevel: "", + ExpectedLogPath: "", }, } for _, tc := range testCases { @@ -84,7 +84,7 @@ func TestParseAllLogLevels(t *testing.T) { assert := assert.New(t) assert.Equal(nil, err, "Error should be nil") - assert.Equal(logLevel, *config.Common.LogLevel, "Log level should be as expected") + assert.Equal(logLevel, config.Common.LogLevel, "Log level should be as expected") }) } } @@ -155,11 +155,6 @@ func TestParseConfigurationFails(t *testing.T) { } } -func toStringPointer(value string) *string { - var copyOfValue = value - return ©OfValue -} - func CreateFile(t *testing.T, fileName string, fileContents string, directory string) string { fullFileName := path.Join(directory, fileName) writeErr := os.WriteFile(fullFileName, []byte(fileContents), 0644) From 67b85713419771e175cb9d6572401fa509913f31 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Thu, 12 Oct 2023 17:59:00 +0200 Subject: [PATCH 08/10] change variable names --- client_configuration.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/client_configuration.go b/client_configuration.go index ea6a637b2..f875b3e0f 100644 --- a/client_configuration.go +++ b/client_configuration.go @@ -35,18 +35,18 @@ func parseClientConfiguration(filePath string) (*ClientConfig, error) { if filePath == "" { return nil, nil } - fileContents, readErr := os.ReadFile(filePath) - if readErr != nil { - return nil, parsingClientConfigError(readErr) + fileContents, err := os.ReadFile(filePath) + if err != nil { + return nil, parsingClientConfigError(err) } var clientConfig ClientConfig - parseErr := json.Unmarshal(fileContents, &clientConfig) - if parseErr != nil { - return nil, parsingClientConfigError(parseErr) + err = json.Unmarshal(fileContents, &clientConfig) + if err != nil { + return nil, parsingClientConfigError(err) } - validateErr := validateClientConfiguration(&clientConfig) - if validateErr != nil { - return nil, parsingClientConfigError(validateErr) + err = validateClientConfiguration(&clientConfig) + if err != nil { + return nil, parsingClientConfigError(err) } return &clientConfig, nil } From ac5b9007f59b187caed5391f0cfb040e4ab05f93 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 13 Oct 2023 10:41:23 +0200 Subject: [PATCH 09/10] revert using assert library --- client_configuration_test.go | 38 +++++++++++++++++++++++------------- go.mod | 5 +---- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/client_configuration_test.go b/client_configuration_test.go index bf8004f29..94480761e 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -4,7 +4,6 @@ package gosnowflake import ( "fmt" - "github.com/stretchr/testify/assert" "os" "path" "strings" @@ -60,10 +59,15 @@ func TestParseConfiguration(t *testing.T) { config, err := parseClientConfiguration(fileName) - assert := assert.New(t) - assert.Equal(nil, err, "Error should be nil") - assert.Equal(tc.ExpectedLogLevel, config.Common.LogLevel, "Log level should be as expected") - assert.Equal(tc.ExpectedLogPath, config.Common.LogPath, "Log path should be as expected") + if err != nil { + t.Fatalf("Error should be nil but was %s", err) + } + if config.Common.LogLevel != tc.ExpectedLogLevel { + t.Errorf("Log level should be %s but was %s", tc.ExpectedLogLevel, config.Common.LogLevel) + } + if config.Common.LogPath != tc.ExpectedLogPath { + t.Errorf("Log path should be %s but was %s", tc.ExpectedLogPath, config.Common.LogPath) + } }) } } @@ -82,9 +86,12 @@ func TestParseAllLogLevels(t *testing.T) { config, err := parseClientConfiguration(fileName) - assert := assert.New(t) - assert.Equal(nil, err, "Error should be nil") - assert.Equal(logLevel, config.Common.LogLevel, "Log level should be as expected") + if err != nil { + t.Fatalf("Error should be nil but was: %s", err) + } + if config.Common.LogLevel != logLevel { + t.Errorf("Log level should be %s but was %s", logLevel, config.Common.LogLevel) + } }) } } @@ -143,14 +150,17 @@ func TestParseConfigurationFails(t *testing.T) { _, err := parseClientConfiguration(fileName) - assert := assert.New(t) - assert.Equal(err != nil, true, "Error should not be nil") + if err == nil { + t.Fatal("Error should not be nil but was nil") + } errMessage := fmt.Sprint(err) expectedPrefix := "parsing client config failed" - assert.Equal(strings.HasPrefix(errMessage, expectedPrefix), true, - fmt.Sprintf("Error message: \"%s\" should start with prefix: \"%s\"", errMessage, expectedPrefix)) - assert.Equal(strings.Contains(errMessage, tc.ExpectedErrorMessageToContain), true, - fmt.Sprintf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, tc.ExpectedErrorMessageToContain)) + if !strings.HasPrefix(errMessage, expectedPrefix) { + t.Errorf("Error message: \"%s\" should start with prefix: \"%s\"", errMessage, expectedPrefix) + } + if !strings.Contains(errMessage, tc.ExpectedErrorMessageToContain) { + t.Errorf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, tc.ExpectedErrorMessageToContain) + } }) } } diff --git a/go.mod b/go.mod index 718e20311..d78792d8f 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,6 @@ require ( github.com/gabriel-vasile/mimetype v1.4.2 github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 github.com/sirupsen/logrus v1.9.0 - github.com/stretchr/testify v1.8.1 golang.org/x/crypto v0.7.0 ) @@ -35,7 +34,6 @@ require ( github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.9.25 // indirect github.com/aws/aws-sdk-go-v2/service/internal/s3shared v1.14.0 // indirect github.com/danieljoos/wincred v1.1.2 // indirect - github.com/davecgh/go-spew v1.1.1 // indirect github.com/dvsekhvalnov/jose2go v1.5.0 // indirect github.com/goccy/go-json v0.10.0 // indirect github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2 // indirect @@ -52,7 +50,7 @@ require ( github.com/minio/c2goasm v0.0.0-20190812172519-36a3d3bbc4f3 // indirect github.com/mtibben/percent v0.2.1 // indirect github.com/pierrec/lz4/v4 v4.1.17 // indirect - github.com/pmezard/go-difflib v1.0.0 // indirect + github.com/stretchr/testify v1.8.1 // indirect github.com/zeebo/xxh3 v1.0.2 // indirect golang.org/x/exp v0.0.0-20230206171751-46f607a40771 // indirect golang.org/x/mod v0.8.0 // indirect @@ -63,5 +61,4 @@ require ( golang.org/x/text v0.8.0 // indirect golang.org/x/tools v0.6.0 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect ) From 5212e1aab3e900d15c4b4c05e4af6e9c258f7920 Mon Sep 17 00:00:00 2001 From: Krzysztof Nozderko Date: Fri, 13 Oct 2023 11:23:39 +0200 Subject: [PATCH 10/10] changes after review --- client_configuration.go | 14 +++--- client_configuration_test.go | 98 ++++++++++++++++++------------------ 2 files changed, 56 insertions(+), 56 deletions(-) diff --git a/client_configuration.go b/client_configuration.go index f875b3e0f..381ed8d86 100644 --- a/client_configuration.go +++ b/client_configuration.go @@ -12,12 +12,12 @@ import ( // log levels for easy logging const ( - Off string = "OFF" // log level for logging switched off - Error string = "ERROR" // error log level - Warn string = "WARN" // warn log level - Info string = "INFO" // info log level - Debug string = "DEBUG" // debug log level - Trace string = "TRACE" // trace log level + levelOff string = "OFF" // log level for logging switched off + levelError string = "ERROR" // error log level + levelWarn string = "WARN" // warn log level + levelInfo string = "INFO" // info log level + levelDebug string = "DEBUG" // debug log level + levelTrace string = "TRACE" // trace log level ) // ClientConfig config root @@ -79,7 +79,7 @@ func validateLogLevel(clientConfig ClientConfig) error { func toLogLevel(logLevelString string) (string, error) { var logLevel = strings.ToUpper(logLevelString) switch logLevel { - case Off, Error, Warn, Info, Debug, Trace: + case levelOff, levelError, levelWarn, levelInfo, levelDebug, levelTrace: return logLevel, nil default: return "", errors.New("unknown log level: " + logLevelString) diff --git a/client_configuration_test.go b/client_configuration_test.go index 94480761e..b7ecdd2f0 100644 --- a/client_configuration_test.go +++ b/client_configuration_test.go @@ -13,60 +13,60 @@ import ( func TestParseConfiguration(t *testing.T) { dir := t.TempDir() testCases := []struct { - Name string - FileName string - FileContents string - ExpectedLogLevel string - ExpectedLogPath string + testName string + fileName string + fileContents string + expectedLogLevel string + expectedLogPath string }{ { - Name: "TestWithLogLevelUpperCase", - FileName: "config_1.json", - FileContents: `{ + testName: "TestWithLogLevelUpperCase", + fileName: "config_1.json", + fileContents: `{ "common": { "log_level" : "INFO", "log_path" : "/some-path/some-directory" } }`, - ExpectedLogLevel: "INFO", - ExpectedLogPath: "/some-path/some-directory", + expectedLogLevel: "INFO", + expectedLogPath: "/some-path/some-directory", }, { - Name: "TestWithLogLevelLowerCase", - FileName: "config_2.json", - FileContents: `{ + testName: "TestWithLogLevelLowerCase", + fileName: "config_2.json", + fileContents: `{ "common": { "log_level" : "info", "log_path" : "/some-path/some-directory" } }`, - ExpectedLogLevel: "info", - ExpectedLogPath: "/some-path/some-directory", + expectedLogLevel: "info", + expectedLogPath: "/some-path/some-directory", }, { - Name: "TestWithMissingValues", - FileName: "config_3.json", - FileContents: `{ + testName: "TestWithMissingValues", + fileName: "config_3.json", + fileContents: `{ "common": {} }`, - ExpectedLogLevel: "", - ExpectedLogPath: "", + expectedLogLevel: "", + expectedLogPath: "", }, } for _, tc := range testCases { - t.Run(tc.Name, func(t *testing.T) { - fileName := CreateFile(t, tc.FileName, tc.FileContents, dir) + t.Run(tc.testName, func(t *testing.T) { + fileName := createFile(t, tc.fileName, tc.fileContents, dir) config, err := parseClientConfiguration(fileName) if err != nil { t.Fatalf("Error should be nil but was %s", err) } - if config.Common.LogLevel != tc.ExpectedLogLevel { - t.Errorf("Log level should be %s but was %s", tc.ExpectedLogLevel, config.Common.LogLevel) + if config.Common.LogLevel != tc.expectedLogLevel { + t.Errorf("Log level should be %s but was %s", tc.expectedLogLevel, config.Common.LogLevel) } - if config.Common.LogPath != tc.ExpectedLogPath { - t.Errorf("Log path should be %s but was %s", tc.ExpectedLogPath, config.Common.LogPath) + if config.Common.LogPath != tc.expectedLogPath { + t.Errorf("Log path should be %s but was %s", tc.expectedLogPath, config.Common.LogPath) } }) } @@ -82,7 +82,7 @@ func TestParseAllLogLevels(t *testing.T) { "log_path" : "/some-path/some-directory" } }`, logLevel) - fileName := CreateFile(t, fmt.Sprintf("config_%s.json", logLevel), fileContents, dir) + fileName := createFile(t, fmt.Sprintf("config_%s.json", logLevel), fileContents, dir) config, err := parseClientConfiguration(fileName) @@ -99,54 +99,54 @@ func TestParseAllLogLevels(t *testing.T) { func TestParseConfigurationFails(t *testing.T) { dir := t.TempDir() testCases := []struct { - Name string - FileName string + testName string + fileName string FileContents string - ExpectedErrorMessageToContain string + expectedErrorMessageToContain string }{ { - Name: "TestWithWrongLogLevel", - FileName: "config_1.json", + testName: "TestWithWrongLogLevel", + fileName: "config_1.json", FileContents: `{ "common": { "log_level" : "something weird", "log_path" : "/some-path/some-directory" } }`, - ExpectedErrorMessageToContain: "unknown log level", + expectedErrorMessageToContain: "unknown log level", }, { - Name: "TestWithWrongTypeOfLogLevel", - FileName: "config_2.json", + testName: "TestWithWrongTypeOfLogLevel", + fileName: "config_2.json", FileContents: `{ "common": { "log_level" : 15, "log_path" : "/some-path/some-directory" } }`, - ExpectedErrorMessageToContain: "ClientConfigCommonProps.common.log_level", + expectedErrorMessageToContain: "ClientConfigCommonProps.common.log_level", }, { - Name: "TestWithWrongTypeOfLogPath", - FileName: "config_3.json", + testName: "TestWithWrongTypeOfLogPath", + fileName: "config_3.json", FileContents: `{ "common": { "log_level" : "INFO", "log_path" : true } }`, - ExpectedErrorMessageToContain: "ClientConfigCommonProps.common.log_path", + expectedErrorMessageToContain: "ClientConfigCommonProps.common.log_path", }, { - Name: "TestWithoutCommon", - FileName: "config_4.json", + testName: "TestWithoutCommon", + fileName: "config_4.json", FileContents: "{}", - ExpectedErrorMessageToContain: "common section in client config not found", + expectedErrorMessageToContain: "common section in client config not found", }, } for _, tc := range testCases { - t.Run(tc.Name, func(t *testing.T) { - fileName := CreateFile(t, tc.FileName, tc.FileContents, dir) + t.Run(tc.testName, func(t *testing.T) { + fileName := createFile(t, tc.fileName, tc.FileContents, dir) _, err := parseClientConfiguration(fileName) @@ -158,17 +158,17 @@ func TestParseConfigurationFails(t *testing.T) { if !strings.HasPrefix(errMessage, expectedPrefix) { t.Errorf("Error message: \"%s\" should start with prefix: \"%s\"", errMessage, expectedPrefix) } - if !strings.Contains(errMessage, tc.ExpectedErrorMessageToContain) { - t.Errorf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, tc.ExpectedErrorMessageToContain) + if !strings.Contains(errMessage, tc.expectedErrorMessageToContain) { + t.Errorf("Error message: \"%s\" should contain given phrase: \"%s\"", errMessage, tc.expectedErrorMessageToContain) } }) } } -func CreateFile(t *testing.T, fileName string, fileContents string, directory string) string { +func createFile(t *testing.T, fileName string, fileContents string, directory string) string { fullFileName := path.Join(directory, fileName) - writeErr := os.WriteFile(fullFileName, []byte(fileContents), 0644) - if writeErr != nil { + err := os.WriteFile(fullFileName, []byte(fileContents), 0644) + if err != nil { t.Fatal("Could not create file") } return fullFileName