usage: return errors and huge simplification

cmd/report.go

@@ -15,9 +15,12 @@ import (
 
 func createReport(
 	u *usage.Usage, execScheduler *execution.Scheduler, importedModules []string, outputs []string,
-) map[string]any {
+) (map[string]any, error) {
 	for _, ec := range execScheduler.GetExecutorConfigs() {
-		u.Uint64("executors/"+ec.GetType(), 1)
+		err := u.Uint64("executors/"+ec.GetType(), 1)
+		if err != nil { // TODO report it as an error but still send the report?
+			return nil, err
+		}
 	}
 
 	// collect the report only with k6 public modules
@@ -33,7 +36,7 @@ func createReport(
 		if !strings.HasPrefix(module, "k6") {
 			continue
 		}
-		u.Strings("modules", module)
+		_ = u.Strings("modules", module) // TODO this will be moved and the error can be logged there
 	}
 
 	builtinOutputs := builtinOutputStrings()
@@ -54,19 +57,23 @@ func createReport(
 		if !builtinOutputsIndex[o] {
 			continue
 		}
-		u.Strings("outputs", o)
+		_ = u.Strings("outputs", o) // TODO this will be moved and the error can be logged there
 	}
 	execState := execScheduler.GetState()
-	u.Uint64("vus_max", uint64(execState.GetInitializedVUsCount()))
-	u.Uint64("iterations", execState.GetFullIterationCount())
-	m := u.Map()
+	m, err := u.Map()
 
-	m["k6_version"] = consts.Version
-	m["duration"] = execState.GetCurrentTestRunDuration().String()
-	m["goos"] = runtime.GOOS
-	m["goarch"] = runtime.GOARCH
+	if m != nil {
+		m["k6_version"] = consts.Version
+		m["duration"] = execState.GetCurrentTestRunDuration().String()
+		m["goos"] = runtime.GOOS
+		m["goarch"] = runtime.GOARCH
+		m["goarch"] = runtime.GOARCH
+		m["goarch"] = runtime.GOARCH
+		m["vus_max"] = uint64(execState.GetInitializedVUsCount())
+		m["iterations"] = execState.GetFullIterationCount()
+	}
 
-	return m
+	return m, err
 }
 
 func reportUsage(ctx context.Context, execScheduler *execution.Scheduler, test *loadedAndConfiguredTest) error {
@@ -76,7 +83,12 @@ func reportUsage(ctx context.Context, execScheduler *execution.Scheduler, test *
 		outputs = append(outputs, outputName)
 	}
 
-	body, err := json.Marshal(createReport(test.usage, execScheduler, test.moduleResolver.Imported(), outputs))
+	m, err := createReport(test.usage, execScheduler, test.moduleResolver.Imported(), outputs)
+	if err != nil {
+		// TODO actually log the error but continue if there is something to report
+		return err
+	}
+	body, err := json.Marshal(m)
 	if err != nil {
 		return err
 	}

cmd/report_test.go

@@ -52,7 +52,8 @@ func TestCreateReport(t *testing.T) {
 	time.Sleep(10 * time.Millisecond)
 	s.GetState().MarkEnded()
 
-	m := createReport(usage.New(), s, importedModules, outputs)
+	m, err := createReport(usage.New(), s, importedModules, outputs)
+	require.NoError(t, err)
 
 	assert.Equal(t, consts.Version, m["k6_version"])
 	assert.Equal(t, map[string]interface{}{"shared-iterations": uint64(1)}, m["executors"])

output/cloud/output.go

@@ -345,7 +345,10 @@ func (out *Output) startVersionedOutput() error {
 	}
 	var err error
 
-	out.usage.Strings("cloud/test_run_id", out.testRunID)
+	usageErr := out.usage.Strings("cloud/test_run_id", out.testRunID)
+	if usageErr != nil {
+		out.logger.Warning("Couldn't report test run id to usage as part of writing to k6 cloud")
+	}
 
 	// TODO: move here the creation of a new cloudapi.Client
 	// so in the case the config has been overwritten the client uses the correct

usage/usage.go

@@ -2,6 +2,9 @@
 package usage
 
 import (
+	"errors"
+	"fmt"
+	"sort"
 	"strings"
 	"sync"
 )
@@ -22,49 +25,54 @@ func New() *Usage {
 
 // Strings appends the provided value to a slice of strings that is the value.
 // Appending to the slice if the key is already there.
-func (u *Usage) Strings(k, v string) {
+func (u *Usage) Strings(k, v string) error {
 	u.l.Lock()
 	defer u.l.Unlock()
 	oldV, ok := u.m[k]
 	if !ok {
 		u.m[k] = []string{v}
-		return
+		return nil
 	}
 	switch oldV := oldV.(type) {
-	case string:
-		u.m[k] = []string{oldV, v}
 	case []string:
 		u.m[k] = append(oldV, v)
 	default:
-		// TODO: error, panic?, nothing, log?
+		return fmt.Errorf("value of key %s is not []string as expected but %T", k, oldV)
 	}
+	return nil
 }
 
 // Uint64 adds the provided value to a given key. Creating the key if needed
-func (u *Usage) Uint64(k string, v uint64) {
+func (u *Usage) Uint64(k string, v uint64) error {
 	u.l.Lock()
 	defer u.l.Unlock()
 	oldV, ok := u.m[k]
 	if !ok {
 		u.m[k] = v
-		return
+		return nil
 	}
-	switch oldV := oldV.(type) {
+	switch oldVUint64 := oldV.(type) {
 	case uint64:
-		u.m[k] = oldV + v
+		u.m[k] = oldVUint64 + v
 	default:
+		return fmt.Errorf("!value of key %s is not uint64 as expected but %T", k, oldV)
 		// TODO: error, panic?, nothing, log?
 	}
+	return nil
 }
 
 // Map returns a copy of the internal map plus making subusages from keys that have a slash in them
 // only a single level is being respected
-func (u *Usage) Map() map[string]any {
+func (u *Usage) Map() (map[string]any, error) {
 	u.l.Lock()
 	defer u.l.Unlock()
+	var errs []error
 
+	keys := mapKeys(u.m)
+	sort.Strings(keys)
 	result := make(map[string]any, len(u.m))
-	for k, v := range u.m {
+	for _, k := range keys {
+		v := u.m[k]
 		prefix, post, found := strings.Cut(k, "/")
 		if !found {
 			result[k] = v
@@ -78,30 +86,20 @@ func (u *Usage) Map() map[string]any {
 		}
 		topLevelMap, ok := topLevel.(map[string]any)
 		if !ok {
-			continue // TODO panic?, error?
-		}
-		keyLevel, ok := topLevelMap[post]
-		switch value := v.(type) {
-		case uint64:
-			switch i := keyLevel.(type) {
-			case uint64:
-				keyLevel = i + value
-			default:
-				// TODO:panic? error?
-			}
-		case []string:
-			switch i := keyLevel.(type) {
-			case []string:
-				keyLevel = append(i, value...) //nolint:gocritic // we assign to the final value
-			default:
-				// TODO:panic? error?
-			}
-		}
-		if !ok {
-			keyLevel = v
+			errs = append(errs, fmt.Errorf("key %s was expected to be a map[string]any but was %T", prefix, topLevel))
+			continue
 		}
-		topLevelMap[post] = keyLevel
+		topLevelMap[post] = v
 	}
 
-	return result
+	return result, errors.Join(errs...)
+}
+
+// replace with map.Keys from go 1.23 after that is the minimal version
+func mapKeys(m map[string]any) []string {
+	keys := make([]string, 0, len(m))
+	for k := range m {
+		keys = append(keys, k)
+	}
+	return keys
 }

usage/usage_test.go

@@ -0,0 +1,40 @@
+package usage
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestErrors(t *testing.T) {
+	t.Parallel()
+	u := New()
+	require.NoError(t, u.Uint64("test/one", 1))
+	require.NoError(t, u.Uint64("test/two", 1))
+	require.NoError(t, u.Uint64("test/two", 1))
+	require.NoError(t, u.Strings("test/three", "three"))
+	require.NoError(t, u.Strings("test2/one", "one"))
+
+	require.ErrorContains(t, u.Strings("test/one", "one"),
+		"test/one is not []string as expected but uint64")
+	require.ErrorContains(t, u.Uint64("test2/one", 1),
+		"test2/one is not uint64 as expected but []string")
+
+	require.NoError(t, u.Strings("test3", "some"))
+	require.NoError(t, u.Strings("test3/one", "one"))
+
+	m, err := u.Map()
+	require.ErrorContains(t, err,
+		"key test3 was expected to be a map[string]any but was []string")
+	require.EqualValues(t, map[string]any{
+		"test": map[string]any{
+			"one":   uint64(1),
+			"two":   uint64(2),
+			"three": []string{"three"},
+		},
+		"test2": map[string]any{
+			"one": []string{"one"},
+		},
+		"test3": []string{"some"},
+	}, m)
+}