fix: Fix config file handle closing and run cross platform unit tests (…

…#639)

## Motivation

The main reason for this PR is to fix config file writing on Windows,
because we were not properly closing the file handle before renaming it,
this cause an error on Windows, stating that file is already used by
another process.

## Summary

In order to better support Linux, MacOS and Windows usages we should run
the unit tests on each of them.

## Release Notes

`sdk.FileConfig` operations which attempt to save the config file should
now operate on Windows without any issues.

.github/workflows/tests.yml

@@ -9,13 +9,18 @@ on:
 jobs:
   test:
     name: Run unit tests
-    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        os: [ubuntu-latest, macos-latest, windows-latest]
+    runs-on: ${{ matrix.os }}
+
     steps:
       - name: Check out code
         uses: actions/checkout@v4
-      - uses: actions/setup-go@v5
+      - name: Install go
+        uses: actions/setup-go@v5
         with:
           go-version-file: go.mod
           cache: false
       - name: Run unit tests
-        run: make test
+        run: go test -race -cover ./... ./docs/mock_example

cspell.yaml

@@ -154,6 +154,7 @@ words:
   - startuml
   - stimeslice
   - strconv
+  - stringutils
   - structs
   - submatches
   - sumologic

internal/manifest/v1alpha/examples/slo_variants.go

@@ -3,7 +3,7 @@ package v1alphaExamples
 import (
 	"embed"
 	"fmt"
-	"path/filepath"
+	"path"
 	"reflect"
 	"slices"
 
@@ -1174,7 +1174,7 @@ func newMetricSpec(metric any) *v1alphaSLO.MetricSpec {
 }
 
 func mustLoadQuery(name string) string {
-	data, err := queriesFS.ReadFile(filepath.Join("queries", name))
+	data, err := queriesFS.ReadFile(path.Join("queries", name))
 	if err != nil {
 		panic(fmt.Sprintf("failed to load query: %s", err))
 	}

internal/stringutils/stringutils.go

@@ -0,0 +1,8 @@
+//go:build !windows
+
+package stringutils
+
+// RemoveCR by default does nothing.
+// It only modifies the string if the binary is built for Windows.
+// See stringutils_windows.go for more details.
+func RemoveCR(s string) string { return s }

internal/stringutils/stringutils_windows.go

@@ -0,0 +1,13 @@
+//go:build windows
+
+package stringutils
+
+import (
+	"strings"
+)
+
+// RemoveCR removes carriage return which is part of the character sequence
+// signifying new line on Windows.
+func RemoveCR(s string) string {
+	return strings.ReplaceAll(s, "\r", "")
+}

manifest/object_test.go

@@ -7,6 +7,8 @@ import (
 
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
+
+	"github.com/nobl9/nobl9-go/internal/stringutils"
 )
 
 func TestFilterByKind(t *testing.T) {
@@ -109,7 +111,11 @@ func TestValidate(t *testing.T) {
 				project: "default"},
 		})
 		assert.Len(t, errs, 1)
-		assert.EqualError(t, errs[0], strings.ReplaceAll(expectedUniquenessConstraintMessage, "\n", "; "))
+		assert.EqualError(
+			t,
+			errs[0],
+			stringutils.RemoveCR(strings.ReplaceAll(expectedUniquenessConstraintMessage, "\n", "; ")),
+		)
 	})
 }
 

manifest/v1alpha/errors_test.go

@@ -3,14 +3,15 @@ package v1alpha
 import (
 	"embed"
 	"encoding/json"
-	"path/filepath"
+	"path"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/nobl9/govy/pkg/govy"
 
+	"github.com/nobl9/nobl9-go/internal/stringutils"
 	"github.com/nobl9/nobl9-go/manifest"
 )
 
@@ -91,7 +92,7 @@ func TestObjectError_UnmarshalJSON(t *testing.T) {
 
 func expectedErrorOutput(t *testing.T, name string) string {
 	t.Helper()
-	data, err := errorsTestData.ReadFile(filepath.Join("test_data", "errors", name))
+	data, err := errorsTestData.ReadFile(path.Join("test_data", "errors", name))
 	require.NoError(t, err)
-	return string(data)
+	return stringutils.RemoveCR(string(data))
 }

manifest/v1alpha/parser/parser_test.go

@@ -3,13 +3,15 @@ package parser
 import (
 	"embed"
 	"encoding/json"
+	"path"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/nobl9/nobl9-go/internal/stringutils"
 	"github.com/nobl9/nobl9-go/manifest"
 	"github.com/nobl9/nobl9-go/manifest/v1alpha"
 	"github.com/nobl9/nobl9-go/manifest/v1alpha/alertmethod"
@@ -118,7 +120,7 @@ func TestParseAlertPolicy(t *testing.T) {
 			jsonData, _ := readParserTestFile(t, testCase.expected)
 			require.NoError(t, err)
 
-			assert.Equal(t, marshalledJson, jsonData)
+			assert.Equal(t, string(marshalledJson), string(jsonData))
 		})
 	}
 }
@@ -147,11 +149,11 @@ func Test_ParseObject_DoubleQuotedJSONHandling(t *testing.T) {
 
 func readParserTestFile(t *testing.T, filename string) ([]byte, manifest.ObjectFormat) {
 	t.Helper()
-	data, err := parserTestData.ReadFile(filepath.Join("test_data", filename))
+	data, err := parserTestData.ReadFile(path.Join("test_data", filename))
 	require.NoError(t, err)
 	format, err := manifest.ParseObjectFormat(filepath.Ext(filename)[1:])
 	require.NoError(t, err)
-	return data, format
+	return []byte(stringutils.RemoveCR(string(data))), format
 }
 
 func validAlertPolicy() alertpolicy.AlertPolicy {

manifest/v1alpha/slo/metrics_cloudwatch_test.go

@@ -2,7 +2,7 @@ package slo
 
 import (
 	"embed"
-	"path/filepath"
+	"path"
 	"strconv"
 	"strings"
 	"testing"
@@ -391,7 +391,7 @@ var testData embed.FS
 
 func getCloudWatchJSON(t *testing.T, name string) *string {
 	t.Helper()
-	data, err := testData.ReadFile(filepath.Join("test_data", name+".json"))
+	data, err := testData.ReadFile(path.Join("test_data", name+".json"))
 	require.NoError(t, err)
 	s := string(data)
 	return &s

sdk/client_test.go

@@ -13,6 +13,7 @@ import (
 	"net/url"
 	"os/exec"
 	"path/filepath"
+	"runtime"
 	"testing"
 	"time"
 
@@ -91,10 +92,19 @@ func TestDefaultUserAgent(t *testing.T) {
 		return ""
 	}
 	tempDir := t.TempDir()
-	path := filepath.Join(tempDir, "test-binary")
+	binName := "test-binary"
+	if runtime.GOOS == "windows" {
+		binName += ".exe"
+	}
+	path := filepath.Join(tempDir, binName)
 	// Build binary. This is the only way for debug package to work,
 	// it needs to operate on a binary built from a module.
-	_, err := exec.Command("go", "build", "-o", path, "./test_data/client/simple_module/main.go").Output()
+	_, err := exec.Command(
+		"go",
+		"build",
+		"-o", path,
+		filepath.FromSlash("./test_data/client/simple_module/main.go"),
+	).Output()
 	require.NoError(t, err, getStderrFromExec(err))
 	// Execute the binary.
 	out, err := exec.Command(path).Output()

sdk/config_file.go

@@ -3,7 +3,6 @@ package sdk
 import (
 	"os"
 	"path/filepath"
-	"syscall"
 
 	"github.com/BurntSushi/toml"
 	"github.com/pkg/errors"
@@ -41,40 +40,31 @@ func (f *FileConfig) Load(path string) error {
 }
 
 // Save saves [FileConfig] into provided path, encoding it in TOML format.
-func (f *FileConfig) Save(path string) (err error) {
-	tmpFile, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path))
+func (f *FileConfig) Save(path string) error {
+	tmpFileName, err := f.writeToTempFile(path)
 	if err != nil {
+		return errors.Wrapf(err, "failed to create and write a temporary config file used for saving the config changes")
+	}
+	if err = os.Rename(tmpFileName, path); err != nil {
 		return err
 	}
+	f.filePath = path
+	return nil
+}
 
-	defer func() {
-		handleFSErr := func(fsErr error) {
-			// If error was encountered in the outer scope or no FS error, return.
-			if err != nil || fsErr == nil {
-				return
-			}
-			if v, isPathErr := fsErr.(*os.PathError); isPathErr &&
-				(v.Err == os.ErrClosed || v.Err == syscall.ENOENT) {
-				return
-			}
-			err = fsErr
-		}
-		// Close and remove temporary file.
-		handleFSErr(tmpFile.Close())
-		handleFSErr(os.Remove(tmpFile.Name()))
-	}()
-
+func (f *FileConfig) writeToTempFile(path string) (tmpFileName string, err error) {
+	tmpFile, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path))
+	if err != nil {
+		return "", err
+	}
+	defer func() { _ = tmpFile.Close() }()
 	if err = toml.NewEncoder(tmpFile).Encode(f); err != nil {
-		return err
+		return "", err
 	}
 	if err = tmpFile.Sync(); err != nil {
-		return err
+		return "", err
 	}
-	if err = os.Rename(tmpFile.Name(), path); err != nil {
-		return err
-	}
-	f.filePath = path
-	return nil
+	return tmpFile.Name(), nil
 }
 
 func createDefaultConfigFile(path string) error {

sdk/config_file_test.go

@@ -98,9 +98,7 @@ func TestFileConfig_Save(t *testing.T) {
 		config := &FileConfig{filePath: filePath}
 
 		err = config.Save(filePath)
-		require.Error(t, err)
-		// Rename will fail with EEXIST errno if the new file is a directory.
-		assert.ErrorIs(t, err, syscall.EEXIST)
+		assert.Error(t, err)
 	})
 
 	t.Run("save config file", func(t *testing.T) {

sdk/config_test.go

@@ -2,12 +2,13 @@ package sdk
 
 import (
 	"embed"
-	_ "embed"
 	"fmt"
 	"io"
 	"net/url"
 	"os"
+	"path"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -47,7 +48,7 @@ func TestReadConfig_FromMinimalConfigFile(t *testing.T) {
 	})
 
 	t.Run("default config file", func(t *testing.T) {
-		t.Setenv("HOME", tempDir)
+		setHomeEnv(t, tempDir)
 		filePath := filepath.Join(tempDir, defaultRelativeConfigPath)
 		copyEmbeddedFile(t, "minimal_config.toml", filePath)
 
@@ -116,7 +117,7 @@ func TestReadConfig_CreateConfigFileIfNotPresent(t *testing.T) {
 
 	t.Run("default config file", func(t *testing.T) {
 		filePath := filepath.Join(tempDir, defaultRelativeConfigPath)
-		t.Setenv("HOME", tempDir)
+		setHomeEnv(t, tempDir)
 		require.NoFileExists(t, filePath)
 
 		conf, err := ReadConfig(ConfigOptionWithCredentials("clientId", "clientSecret"))
@@ -194,7 +195,7 @@ func TestReadConfig_Defaults(t *testing.T) {
 func TestReadConfig_EnvVariablesMinimal(t *testing.T) {
 	// So that we don't run into conflicts with existing config.toml.
 	tempDir := t.TempDir()
-	t.Setenv("HOME", tempDir)
+	setHomeEnv(t, tempDir)
 
 	for k, v := range map[string]string{
 		"NO_CONFIG_FILE": "true",
@@ -228,7 +229,7 @@ func TestReadConfig_EnvVariablesFull(t *testing.T) {
 	tempDir := setupConfigTestData(t)
 	filePath := filepath.Join(tempDir, "full_config_env_override.toml")
 	// So that we don't run into conflicts with existing config.toml.
-	t.Setenv("HOME", tempDir)
+	setHomeEnv(t, tempDir)
 
 	for _, envPrefix := range []string{EnvPrefix, "MY_PREFIX_", ""} {
 		for k, v := range map[string]string{
@@ -418,7 +419,7 @@ func setupConfigTestData(t *testing.T) (tempDir string) {
 }
 
 func copyEmbeddedFile(t *testing.T, sourceName, dest string) {
-	embeddedFile, err := configTestData.Open(filepath.Join(configTestDataPath, sourceName))
+	embeddedFile, err := configTestData.Open(path.Join(configTestDataPath, sourceName))
 	require.NoError(t, err)
 	defer func() { _ = embeddedFile.Close() }()
 
@@ -432,3 +433,11 @@ func copyEmbeddedFile(t *testing.T, sourceName, dest string) {
 	_, err = io.Copy(tmpFile, embeddedFile)
 	require.NoError(t, err)
 }
+
+func setHomeEnv(t *testing.T, homePath string) {
+	envKey := "HOME"
+	if runtime.GOOS == "windows" {
+		envKey = "USERPROFILE"
+	}
+	t.Setenv(envKey, homePath)
+}

sdk/decode_test.go

@@ -5,6 +5,7 @@ import (
 	"embed"
 	"fmt"
 	"os"
+	"path"
 	"path/filepath"
 	"testing"
 
@@ -198,7 +199,7 @@ func TestDecodeSingle(t *testing.T) {
 
 func readInputFile(t *testing.T, name string) []byte {
 	t.Helper()
-	data, err := decodeTestData.ReadFile(filepath.Join("test_data", "decode", name))
+	data, err := decodeTestData.ReadFile(path.Join("test_data", "decode", name))
 	require.NoError(t, err)
 	return data
 }