From e51cfe276c029a3954bae84f0ef85fba7cf085d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Stephan=20A=C3=9Fmus?= Date: Tue, 17 Mar 2020 08:33:35 +0100 Subject: [PATCH] Fix copy-paste-bug for downloading settings (#1284) * Fix copy-paste-bug for downloading settings * Extend unit tests accordingly. * Fix some expected versus actual mixup --- pkg/maven/maven.go | 51 +++++++++++++++++---------- pkg/maven/maven_test.go | 77 +++++++++++++++++++++-------------------- 2 files changed, 71 insertions(+), 57 deletions(-) diff --git a/pkg/maven/maven.go b/pkg/maven/maven.go index 0eca9b67a..483e0c77b 100644 --- a/pkg/maven/maven.go +++ b/pkg/maven/maven.go @@ -39,14 +39,15 @@ func Execute(options *ExecuteOptions, command mavenExecRunner) (string, error) { command.Stdout(stdOut) command.Stderr(log.Entry().Writer()) - parameters := getParametersFromOptions(options, &http.Client{}) - - err := command.RunExecutable(mavenExecutable, parameters...) + parameters, err := getParametersFromOptions(options, &http.Client{}) if err != nil { - log.Entry(). - WithError(err). - WithField("command", append([]string{mavenExecutable}, parameters...)). - Fatal("failed to execute run command") + return "", fmt.Errorf("failed to construct parameters from options: %w", err) + } + + err = command.RunExecutable(mavenExecutable, parameters...) + if err != nil { + commandLine := append([]string{mavenExecutable}, parameters...) + return "", fmt.Errorf("failed to run executable, command: '%s', error: %w", commandLine, err) } if stdOutBuf == nil { @@ -88,23 +89,33 @@ func evaluateStdOut(config *ExecuteOptions) (*bytes.Buffer, io.Writer) { return stdOutBuf, stdOut } -func getParametersFromOptions(options *ExecuteOptions, client http.Downloader) []string { +func downloadSettingsIfURL(settingsFileOption, settingsFile string, client http.Downloader) (string, error) { + result := settingsFileOption + if strings.HasPrefix(settingsFileOption, "http:") || strings.HasPrefix(settingsFileOption, "https:") { + err := downloadSettingsFromURL(settingsFileOption, settingsFile, client) + if err != nil { + return "", err + } + result = settingsFile + } + return result, nil +} + +func getParametersFromOptions(options *ExecuteOptions, client http.Downloader) ([]string, error) { var parameters []string if options.GlobalSettingsFile != "" { - globalSettingsFileName := options.GlobalSettingsFile - if strings.HasPrefix(options.GlobalSettingsFile, "http:") || strings.HasPrefix(options.GlobalSettingsFile, "https:") { - downloadSettingsFromURL(options.ProjectSettingsFile, "globalSettings.xml", client) - globalSettingsFileName = "globalSettings.xml" + globalSettingsFileName, err := downloadSettingsIfURL(options.GlobalSettingsFile, "globalSettings.xml", client) + if err != nil { + return nil, err } parameters = append(parameters, "--global-settings", globalSettingsFileName) } if options.ProjectSettingsFile != "" { - projectSettingsFileName := options.ProjectSettingsFile - if strings.HasPrefix(options.ProjectSettingsFile, "http:") || strings.HasPrefix(options.ProjectSettingsFile, "https:") { - downloadSettingsFromURL(options.ProjectSettingsFile, "projectSettings.xml", client) - projectSettingsFileName = "projectSettings.xml" + projectSettingsFileName, err := downloadSettingsIfURL(options.ProjectSettingsFile, "projectSettings.xml", client) + if err != nil { + return nil, err } parameters = append(parameters, "--settings", projectSettingsFileName) } @@ -132,15 +143,17 @@ func getParametersFromOptions(options *ExecuteOptions, client http.Downloader) [ } parameters = append(parameters, options.Goals...) - return parameters + return parameters, nil } // ToDo replace with pkg/maven/settings GetSettingsFile -func downloadSettingsFromURL(url, filename string, client http.Downloader) { +func downloadSettingsFromURL(url, filename string, client http.Downloader) error { err := client.DownloadFile(url, filename, nil, nil) if err != nil { - log.Entry().WithError(err).Fatal("Failed to download maven settings from: " + url) + return fmt.Errorf("failed to download maven settings from URL '%s' to file '%s': %w", + url, filename, err) } + return nil } func GetTestModulesExcludes() []string { diff --git a/pkg/maven/maven_test.go b/pkg/maven/maven_test.go index dd3b00a04..ec90b473e 100644 --- a/pkg/maven/maven_test.go +++ b/pkg/maven/maven_test.go @@ -10,10 +10,28 @@ import ( "testing" piperHttp "github.com/SAP/jenkins-library/pkg/http" - "github.com/SAP/jenkins-library/pkg/log" "github.com/stretchr/testify/assert" ) +type mockDownloader struct { + shouldFail bool + requestedUrls []string + requestedFiles []string +} + +func (m *mockDownloader) DownloadFile(url, filename string, header http.Header, cookies []*http.Cookie) error { + m.requestedUrls = append(m.requestedUrls, url) + m.requestedFiles = append(m.requestedFiles, filename) + if m.shouldFail { + return errors.New("something happened") + } + return nil +} + +func (m *mockDownloader) SetOptions(options piperHttp.ClientOptions) { + return +} + func TestExecute(t *testing.T) { t.Run("should return stdOut", func(t *testing.T) { expectedOutput := "mocked output" @@ -36,15 +54,13 @@ func TestExecute(t *testing.T) { assert.Equal(t, expectedOutput, mavenOutput) }) t.Run("should log that command failed if executing maven failed", func(t *testing.T) { - var hasFailed bool - log.Entry().Logger.ExitFunc = func(int) { hasFailed = true } execMockRunner := mock.ExecMockRunner{ShouldFailOnCommand: map[string]error{"mvn --file pom.xml --batch-mode": errors.New("error case")}} opts := ExecuteOptions{PomPath: "pom.xml", ReturnStdout: false} - output, _ := Execute(&opts, &execMockRunner) + output, err := Execute(&opts, &execMockRunner) - assert.True(t, hasFailed, "failed to execute run command") - assert.Equal(t, output, "") + assert.EqualError(t, err, "failed to run executable, command: '[mvn --file pom.xml --batch-mode]', error: error case") + assert.Equal(t, "", output) }) t.Run("should have all configured parameters in the exec call", func(t *testing.T) { execMockRunner := mock.ExecMockRunner{} @@ -59,8 +75,8 @@ func TestExecute(t *testing.T) { mavenOutput, _ := Execute(&opts, &execMockRunner) - assert.Equal(t, len(execMockRunner.Calls[0].Params), len(expectedParameters)) - assert.Equal(t, execMockRunner.Calls[0], mock.ExecCall{Exec: "mvn", Params: expectedParameters}) + assert.Equal(t, len(expectedParameters), len(execMockRunner.Calls[0].Params)) + assert.Equal(t, mock.ExecCall{Exec: "mvn", Params: expectedParameters}, execMockRunner.Calls[0]) assert.Equal(t, "", mavenOutput) }) } @@ -86,51 +102,36 @@ func TestEvaluate(t *testing.T) { }) } -type mockDownloader struct { - shouldFail bool -} - -func (m *mockDownloader) DownloadFile(url, filename string, header http.Header, cookies []*http.Cookie) error { - if m.shouldFail { - return errors.New("something happened") - } - return nil -} - -func (m *mockDownloader) SetOptions(options piperHttp.ClientOptions) { - return -} - func TestGetParameters(t *testing.T) { t.Run("should resolve configured parameters and download the settings files", func(t *testing.T) { mockClient := mockDownloader{shouldFail: false} opts := ExecuteOptions{PomPath: "pom.xml", GlobalSettingsFile: "https://mysettings.com", ProjectSettingsFile: "http://myprojectsettings.com", ReturnStdout: false} expectedParameters := []string{"--global-settings", "globalSettings.xml", "--settings", "projectSettings.xml", "--file", "pom.xml", "--batch-mode"} - parameters := getParametersFromOptions(&opts, &mockClient) - - assert.Equal(t, len(parameters), len(expectedParameters)) - assert.Equal(t, parameters, expectedParameters) + parameters, err := getParametersFromOptions(&opts, &mockClient) + if assert.NoError(t, err) { + assert.Equal(t, len(expectedParameters), len(parameters)) + assert.Equal(t, expectedParameters, parameters) + if assert.Equal(t, 2, len(mockClient.requestedUrls)) { + assert.Equal(t, "https://mysettings.com", mockClient.requestedUrls[0]) + assert.Equal(t, "globalSettings.xml", mockClient.requestedFiles[0]) + assert.Equal(t, "http://myprojectsettings.com", mockClient.requestedUrls[1]) + assert.Equal(t, "projectSettings.xml", mockClient.requestedFiles[1]) + } + } }) } func TestDownloadSettingsFromURL(t *testing.T) { t.Run("should pass if download is successful", func(t *testing.T) { - var hasFailed bool - log.Entry().Logger.ExitFunc = func(int) { hasFailed = true } mockClient := mockDownloader{shouldFail: false} - - downloadSettingsFromURL("anyURL", "settings.xml", &mockClient) - - assert.False(t, hasFailed) + err := downloadSettingsFromURL("anyURL", "settings.xml", &mockClient) + assert.NoError(t, err) }) t.Run("should fail if download fails", func(t *testing.T) { - var hasFailed bool - log.Entry().Logger.ExitFunc = func(int) { hasFailed = true } mockClient := mockDownloader{shouldFail: true} - - downloadSettingsFromURL("anyURL", "settings.xml", &mockClient) - assert.True(t, hasFailed, "expected command to exit with fatal") + err := downloadSettingsFromURL("anyURL", "settings.xml", &mockClient) + assert.EqualError(t, err, "failed to download maven settings from URL 'anyURL' to file 'settings.xml': something happened") }) }