Repository: osbuild/osbuild-composer
Pull request #4192 (2024-06-06 08:29:50)

osbuild-worker-executor: fix order of `assert.Equal()` in tests
The `assert.Equal()` expects that the "expected" value is put first. Which is not what I'm used to. It's also slightly inconsistent because `assert.EqualError()` expects the "actual" err first and then the expected string. But this commit is not about ranting :)

This commit fixes the order in the tests assert.Equal() so that mismatches actually are displayed correctly.




Patch:
diff --git a/cmd/osbuild-worker-executor/handler_build.go b/cmd/osbuild-worker-executor/handler_build.go
index a790ae523..096876ab5 100644
--- a/cmd/osbuild-worker-executor/handler_build.go
+++ b/cmd/osbuild-worker-executor/handler_build.go
@@ -39,7 +39,7 @@ func (wf *writeFlusher) Write(p []byte) (n int, err error) {
 	return n, err
 }
 
-func runOsbuild(buildDir string, control *controlJSON, output io.Writer) (string, error) {
+func runOsbuild(logger *logrus.Logger, buildDir string, control *controlJSON, output io.Writer) (string, error) {
 	flusher, ok := output.(http.Flusher)
 	if !ok {
 		return "", fmt.Errorf("cannot stream the output")
@@ -92,11 +92,11 @@ func runOsbuild(buildDir string, control *controlJSON, output io.Writer) (string
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		err = fmt.Errorf("cannot tar output directory: %w, output:\n%s", err, out)
-		logrus.Errorf(err.Error())
+		logger.Errorf(err.Error())
 		_, _ = mw.Write([]byte(err.Error()))
 		return "", err
 	}
-	logrus.Infof("tar output:\n%s", out)
+	logger.Infof("tar output:\n%s", out)
 	return outputDir, nil
 }
 
@@ -286,7 +286,7 @@ func handleBuild(logger *logrus.Logger, config *Config) http.Handler {
 
 			// run osbuild and stream the output to the client
 			buildResult := newBuildResult(config)
-			_, err = runOsbuild(buildDir, control, w)
+			_, err = runOsbuild(logger, buildDir, control, w)
 			if werr := buildResult.Mark(err); werr != nil {
 				logger.Errorf("cannot write result file %v", werr)
 			}
diff --git a/cmd/osbuild-worker-executor/main.go b/cmd/osbuild-worker-executor/main.go
index 59edfcbc9..2330ea9c8 100644
--- a/cmd/osbuild-worker-executor/main.go
+++ b/cmd/osbuild-worker-executor/main.go
@@ -16,8 +16,6 @@ import (
 // based on the excellent post
 // https://grafana.com/blog/2024/02/09/how-i-write-http-services-in-go-after-13-years/
 
-var logrusNew = logrus.New
-
 func newServer(logger *logrus.Logger, config *Config) http.Handler {
 	mux := http.NewServeMux()
 	addRoutes(mux, logger, config)
@@ -27,11 +25,10 @@ func newServer(logger *logrus.Logger, config *Config) http.Handler {
 	return handler
 }
 
-func run(ctx context.Context, args []string, getenv func(string) string) error {
+func run(ctx context.Context, args []string, getenv func(string) string, logger *logrus.Logger) error {
 	ctx, cancel := signal.NotifyContext(ctx, os.Interrupt)
 	defer cancel()
 
-	logger := logrusNew()
 	config, err := newConfigFromCmdline(args)
 	if err != nil {
 		return err
@@ -75,8 +72,9 @@ func run(ctx context.Context, args []string, getenv func(string) string) error {
 }
 
 func main() {
+	logger := logrus.New()
 	ctx := context.Background()
-	if err := run(ctx, os.Args, os.Getenv); err != nil {
+	if err := run(ctx, os.Args, os.Getenv, logger); err != nil {
 		fmt.Fprintf(os.Stderr, "%s\n", err)
 		os.Exit(1)
 	}
diff --git a/cmd/osbuild-worker/jobimpl-depsolve.go b/cmd/osbuild-worker/jobimpl-depsolve.go
index f50bd209a..4dbc5495e 100644
--- a/cmd/osbuild-worker/jobimpl-depsolve.go
+++ b/cmd/osbuild-worker/jobimpl-depsolve.go
@@ -90,7 +90,7 @@ func (impl *DepsolveJobImpl) Run(job worker.Job) error {
 					for _, baseurlstr := range repo.BaseURLs {
 						match, err := impl.RepositoryMTLSConfig.CompareBaseURL(baseurlstr)
 						if err != nil {
-							result.JobError = clienterrors.WorkerClientError(clienterrors.ErrorInvalidRepositoryURL, "Repository URL is malformed", err)
+							result.JobError = clienterrors.WorkerClientError(clienterrors.ErrorInvalidRepositoryURL, "Repository URL is malformed", err.Error())
 							return err
 						}
 						if match {
diff --git a/cmd/osbuild-worker/jobimpl-osbuild.go b/cmd/osbuild-worker/jobimpl-osbuild.go
index 4dde72ce0..6f3930cbb 100644
--- a/cmd/osbuild-worker/jobimpl-osbuild.go
+++ b/cmd/osbuild-worker/jobimpl-osbuild.go
@@ -401,7 +401,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 
 	osbuildVersion, err := osbuild.OSBuildVersion()
 	if err != nil {
-		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "Error getting osbuild binary version", err)
+		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "Error getting osbuild binary version", err.Error())
 		return err
 	}
 	osbuildJobResult.OSBuildVersion = osbuildVersion
@@ -516,7 +516,7 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 	osbuildJobResult.OSBuildOutput, err = executor.RunOSBuild(jobArgs.Manifest, impl.Store, outputDirectory, exports, exportPaths, nil, extraEnv, true, os.Stderr)
 	// First handle the case when "running" osbuild failed
 	if err != nil {
-		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", err)
+		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", err.Error())
 		return err
 	}
 
@@ -544,13 +544,13 @@ func (impl *OSBuildJobImpl) Run(job worker.Job) error {
 
 	// Second handle the case when the build failed, but osbuild finished successfully
 	if !osbuildJobResult.OSBuildOutput.Success {
-		var osbErrors []error
+		var osbErrors []string
 		if osbuildJobResult.OSBuildOutput.Error != nil {
-			osbErrors = append(osbErrors, fmt.Errorf("osbuild error: %s", string(osbuildJobResult.OSBuildOutput.Error)))
+			osbErrors = append(osbErrors, fmt.Sprintf("osbuild error: %s", string(osbuildJobResult.OSBuildOutput.Error)))
 		}
 		if osbuildJobResult.OSBuildOutput.Errors != nil {
 			for _, err := range osbuildJobResult.OSBuildOutput.Errors {
-				osbErrors = append(osbErrors, fmt.Errorf("manifest validation error: %v", err))
+				osbErrors = append(osbErrors, fmt.Sprintf("manifest validation error: %v", err))
 			}
 		}
 		osbuildJobResult.JobError = clienterrors.WorkerClientError(clienterrors.ErrorBuildJob, "osbuild build failed", osbErrors)


Test patch:
diff --git a/cmd/osbuild-worker-executor/export_test.go b/cmd/osbuild-worker-executor/export_test.go
index cf9b430d4..9698623c8 100644
--- a/cmd/osbuild-worker-executor/export_test.go
+++ b/cmd/osbuild-worker-executor/export_test.go
@@ -4,9 +4,6 @@ import (
 	"os"
 	"path/filepath"
 	"testing"
-
-	"github.com/sirupsen/logrus"
-	logrusTest "github.com/sirupsen/logrus/hooks/test"
 )
 
 var (
@@ -15,19 +12,6 @@ var (
 	HandleIncludedSources = handleIncludedSources
 )
 
-func MockLogger() (hook *logrusTest.Hook, restore func()) {
-	saved := logrusNew
-	logger, hook := logrusTest.NewNullLogger()
-	logrusNew = func() *logrus.Logger {
-		return logger
-	}
-	logger.SetLevel(logrus.DebugLevel)
-
-	return hook, func() {
-		logrusNew = saved
-	}
-}
-
 func MockOsbuildBinary(t *testing.T, new string) (restore func()) {
 	t.Helper()
 
diff --git a/cmd/osbuild-worker-executor/handler_build_test.go b/cmd/osbuild-worker-executor/handler_build_test.go
index c89558bcb..302506163 100644
--- a/cmd/osbuild-worker-executor/handler_build_test.go
+++ b/cmd/osbuild-worker-executor/handler_build_test.go
@@ -25,8 +25,8 @@ func TestBuildMustPOST(t *testing.T) {
 	rsp, err := http.Get(endpoint)
 	assert.NoError(t, err)
 	defer rsp.Body.Close()
-	assert.Equal(t, rsp.StatusCode, 405)
-	assert.Equal(t, loggerHook.LastEntry().Message, "handlerBuild called on /api/v1/build")
+	assert.Equal(t, 405, rsp.StatusCode)
+	assert.Equal(t, "handlerBuild called on /api/v1/build", loggerHook.LastEntry().Message)
 }
 
 func writeToTar(atar *tar.Writer, name, content string) error {
@@ -49,10 +49,10 @@ func TestBuildChecksContentType(t *testing.T) {
 	rsp, err := http.Post(endpoint, "random/encoding", nil)
 	assert.NoError(t, err)
 	defer rsp.Body.Close()
-	assert.Equal(t, rsp.StatusCode, http.StatusUnsupportedMediaType)
+	assert.Equal(t, http.StatusUnsupportedMediaType, rsp.StatusCode)
 	body, err := io.ReadAll(rsp.Body)
 	assert.NoError(t, err)
-	assert.Equal(t, string(body), "Content-Type must be [application/x-tar], got random/encoding\n")
+	assert.Equal(t, "Content-Type must be [application/x-tar], got random/encoding\n", string(body))
 }
 
 func makeTestPost(t *testing.T, controlJSON, manifestJSON string) *bytes.Buffer {
@@ -105,7 +105,7 @@ echo "fake-build-result" > %[1]s/build/output/image/disk.img
 	defer func() { _, _ = io.ReadAll(rsp.Body) }()
 	defer rsp.Body.Close()
 
-	assert.Equal(t, rsp.StatusCode, http.StatusCreated)
+	assert.Equal(t, http.StatusCreated, rsp.StatusCode)
 	reader := bufio.NewReader(rsp.Body)
 
 	// check that we get the output of osbuild streamed to us
@@ -114,7 +114,7 @@ echo "fake-build-result" > %[1]s/build/output/image/disk.img
 {"fake": "manifest"}`, baseBuildDir)
 	content, err := io.ReadAll(reader)
 	assert.NoError(t, err)
-	assert.Equal(t, string(content), expectedContent)
+	assert.Equal(t, expectedContent, string(content))
 	// check log too
 	logFileContent, err := os.ReadFile(filepath.Join(baseBuildDir, "build/build.log"))
 	assert.NoError(t, err)
@@ -164,7 +164,7 @@ echo "fake-build-result" > %[1]s/build/output/image/disk.img
 	buf := makeTestPost(t, `{"exports": ["tree"]}`, `{"fake": "manifest"}`)
 	rsp, err := http.Post(endpoint, "application/x-tar", buf)
 	assert.NoError(t, err)
-	assert.Equal(t, rsp.StatusCode, http.StatusCreated)
+	assert.Equal(t, http.StatusCreated, rsp.StatusCode)
 	defer func() { _, _ = io.ReadAll(rsp.Body) }()
 	defer rsp.Body.Close()
 
@@ -172,8 +172,8 @@ echo "fake-build-result" > %[1]s/build/output/image/disk.img
 	rsp, err = http.Post(endpoint, "application/x-tar", buf)
 	assert.NoError(t, err)
 	defer rsp.Body.Close()
-	assert.Equal(t, rsp.StatusCode, http.StatusConflict)
-	assert.Equal(t, loggerHook.LastEntry().Message, main.ErrAlreadyBuilding.Error())
+	assert.Equal(t, http.StatusConflict, rsp.StatusCode)
+	assert.Equal(t, main.ErrAlreadyBuilding.Error(), loggerHook.LastEntry().Message)
 }
 
 func TestHandleIncludedSourcesUnclean(t *testing.T) {
@@ -236,7 +236,7 @@ exit 23
 	defer func() { _, _ = io.ReadAll(rsp.Body) }()
 	defer rsp.Body.Close()
 
-	assert.Equal(t, rsp.StatusCode, http.StatusCreated)
+	assert.Equal(t, http.StatusCreated, rsp.StatusCode)
 	reader := bufio.NewReader(rsp.Body)
 	content, err := io.ReadAll(reader)
 	assert.NoError(t, err)
@@ -280,7 +280,7 @@ echo "fake-build-result" > %[1]s/build/output/image/disk.img
 	defer func() { _, _ = io.ReadAll(rsp.Body) }()
 	defer rsp.Body.Close()
 
-	assert.Equal(t, rsp.StatusCode, http.StatusCreated)
+	assert.Equal(t, http.StatusCreated, rsp.StatusCode)
 	reader := bufio.NewReader(rsp.Body)
 
 	var lineno, seconds, nano int64
diff --git a/cmd/osbuild-worker-executor/handler_result_test.go b/cmd/osbuild-worker-executor/handler_result_test.go
index ccc75a893..287c563aa 100644
--- a/cmd/osbuild-worker-executor/handler_result_test.go
+++ b/cmd/osbuild-worker-executor/handler_result_test.go
@@ -16,7 +16,7 @@ func TestResultTooEarly(t *testing.T) {
 
 	rsp, err := http.Get(endpoint)
 	assert.NoError(t, err)
-	assert.Equal(t, rsp.StatusCode, http.StatusTooEarly)
+	assert.Equal(t, http.StatusTooEarly, rsp.StatusCode)
 }
 
 func TestResultBad(t *testing.T) {
diff --git a/cmd/osbuild-worker-executor/handler_root_test.go b/cmd/osbuild-worker-executor/handler_root_test.go
index da6200238..6a0737d3e 100644
--- a/cmd/osbuild-worker-executor/handler_root_test.go
+++ b/cmd/osbuild-worker-executor/handler_root_test.go
@@ -13,6 +13,6 @@ func TestTrivialRootEndpoint(t *testing.T) {
 	endpoint := baseURL
 	resp, err := http.Get(endpoint)
 	assert.NoError(t, err)
-	assert.Equal(t, resp.StatusCode, 200)
-	assert.Equal(t, loggerHook.LastEntry().Message, "/ handler called")
+	assert.Equal(t, 200, resp.StatusCode)
+	assert.Equal(t, "/ handler called", loggerHook.LastEntry().Message)
 }
diff --git a/cmd/osbuild-worker-executor/main_test.go b/cmd/osbuild-worker-executor/main_test.go
index 50ff90446..97aca273f 100644
--- a/cmd/osbuild-worker-executor/main_test.go
+++ b/cmd/osbuild-worker-executor/main_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/sirupsen/logrus"
 	logrusTest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/assert"
 
@@ -57,8 +58,8 @@ func runTestServer(t *testing.T) (baseURL, buildBaseDir string, loggerHook *logr
 	ctx, cancel := context.WithCancel(context.Background())
 	t.Cleanup(cancel)
 
-	loggerHook, restore := main.MockLogger()
-	defer restore()
+	logger, loggerHook := logrusTest.NewNullLogger()
+	logger.SetLevel(logrus.DebugLevel)
 
 	args := []string{
 		"-host", host,
@@ -66,7 +67,7 @@ func runTestServer(t *testing.T) (baseURL, buildBaseDir string, loggerHook *logr
 		"-build-path", buildBaseDir,
 	}
 	go func() {
-		_ = main.Run(ctx, args, os.Getenv)
+		_ = main.Run(ctx, args, os.Getenv, logger)
 	}()
 
 	err := waitReady(ctx, defaultTimeout, baseURL)