WIP

coder · ethanndickson · Jun 5, 2024 · Jun 3, 2024 · Jun 4, 2024 · Jun 4, 2024
commit 451180c472b43c4878c653caa27410d21710195c
diff --git a/cli/cliui/output.go b/cli/cliui/output.go
@@ -7,6 +7,7 @@ import (
 	"reflect"
 	"strings"
 
+	"github.com/jedib0t/go-pretty/v6/table"
 	"golang.org/x/xerrors"
 
 	"github.com/coder/serpent"
@@ -143,7 +144,11 @@ func (f *tableFormat) AttachOptions(opts *serpent.OptionSet) {
 
 // Format implements OutputFormat.
 func (f *tableFormat) Format(_ context.Context, data any) (string, error) {
-	return renderTable(data, f.sort, f.columns, nil)
+	headers := make(table.Row, len(f.allColumns))
+	for i, header := range f.allColumns {
+		headers[i] = header
+	}
+	return renderTable(data, f.sort, headers, f.columns)
 }
 
 type jsonFormat struct{}

diff --git a/cli/cliui/table.go b/cli/cliui/table.go
@@ -57,7 +57,7 @@ func filterTableColumns(header table.Row, columns []string) []table.ColumnConfig
 // DisplayTable renders a table as a string. The input argument can be:
 //   - a struct slice.
 //   - an interface slice, where the first element is a struct,
-//     and all other elements are of the same type, or a TableSeperator.
+//     and all other elements are of the same type, or a TableSeparator.
 //
 // At least one field in the struct must have a `table:""` tag
 // containing the name of the column in the outputted table.
@@ -77,12 +77,12 @@ func DisplayTable(out any, sort string, filterColumns []string) (string, error)
 	v := reflect.Indirect(reflect.ValueOf(out))
 
 	if v.Kind() != reflect.Slice {
-		return "", xerrors.Errorf("DisplayTable called with a non-slice type")
+		return "", xerrors.New("DisplayTable called with a non-slice type")
 	}
 	var tableType reflect.Type
 	if v.Type().Elem().Kind() == reflect.Interface {
 		if v.Len() == 0 {
-			return "", xerrors.Errorf("DisplayTable called with empty interface slice")
+			return "", xerrors.New("DisplayTable called with empty interface slice")
 		}
 		tableType = reflect.Indirect(reflect.ValueOf(v.Index(0).Interface())).Type()
 	} else {
@@ -100,6 +100,10 @@ func DisplayTable(out any, sort string, filterColumns []string) (string, error)
 	if sort == "" {
 		sort = defaultSort
 	}
+	headers := make(table.Row, len(headersRaw))
+	for i, header := range headersRaw {
+		headers[i] = strings.ReplaceAll(header, "_", " ")
+	}
 	// Verify that the given sort column and filter columns are valid.
 	if sort != "" || len(filterColumns) != 0 {
 		headersMap := make(map[string]string, len(headersRaw))
@@ -145,16 +149,12 @@ func DisplayTable(out any, sort string, filterColumns []string) (string, error)
 			return "", xerrors.Errorf("specified sort column %q not found in table headers, available columns are %q", sort, strings.Join(headersRaw, `", "`))
 		}
 	}
-	return renderTable(out, sort, headersRaw, filterColumns)
+	return renderTable(out, sort, headers, filterColumns)
 }
 
-func renderTable(out any, sort string, headersRaw []string, filterColumns []string) (string, error) {
+func renderTable(out any, sort string, headers table.Row, filterColumns []string) (string, error) {
 	v := reflect.Indirect(reflect.ValueOf(out))
 
-	headers := make(table.Row, len(headersRaw))
-	for i, header := range headersRaw {
-		headers[i] = header
-	}
 	// Setup the table formatter.
 	tw := Table()
 	tw.AppendHeader(headers)
@@ -181,8 +181,8 @@ func renderTable(out any, sort string, headersRaw []string, filterColumns []stri
 		}
 
 		rowSlice := make([]any, len(headers))
-		for i, h := range headersRaw {
-			v, ok := rowMap[h]
+		for i, h := range headers {
+			v, ok := rowMap[h.(string)]
 			if !ok {
 				v = nil
 			}
@@ -219,25 +219,28 @@ func renderTable(out any, sort string, headersRaw []string, filterColumns []stri
 // returned. If the table tag is malformed, an error is returned.
 //
 // The returned name is transformed from "snake_case" to "normal text".
-func parseTableStructTag(field reflect.StructField) (name string, defaultSort, recursive bool, skipParentName bool, err error) {
+func parseTableStructTag(field reflect.StructField) (name string, defaultSort, noSortOpt, recursive, skipParentName bool, err error) {
 	tags, err := structtag.Parse(string(field.Tag))
 	if err != nil {
-		return "", false, false, false, xerrors.Errorf("parse struct field tag %q: %w", string(field.Tag), err)
+		return "", false, false, false, false, xerrors.Errorf("parse struct field tag %q: %w", string(field.Tag), err)
 	}
 
 	tag, err := tags.Get("table")
 	if err != nil || tag.Name == "-" {
 		// tags.Get only returns an error if the tag is not found.
-		return "", false, false, false, nil
+		return "", false, false, false, false, nil
 	}
 
 	defaultSortOpt := false
+	noSortOpt = false
 	recursiveOpt := false
 	skipParentNameOpt := false
 	for _, opt := range tag.Options {
 		switch opt {
 		case "default_sort":
 			defaultSortOpt = true
+		case "nosort":
+			noSortOpt = true
 		case "recursive":
 			recursiveOpt = true
 		case "recursive_inline":
@@ -247,11 +250,11 @@ func parseTableStructTag(field reflect.StructField) (name string, defaultSort, r
 			recursiveOpt = true
 			skipParentNameOpt = true
 		default:
-			return "", false, false, false, xerrors.Errorf("unknown option %q in struct field tag", opt)
+			return "", false, false, false, false, xerrors.Errorf("unknown option %q in struct field tag", opt)
 		}
 	}
 
-	return strings.ReplaceAll(tag.Name, "_", " "), defaultSortOpt, recursiveOpt, skipParentNameOpt, nil
+	return strings.ReplaceAll(tag.Name, "_", " "), defaultSortOpt, noSortOpt, recursiveOpt, skipParentNameOpt, nil
 }
 
 func isStructOrStructPointer(t reflect.Type) bool {
@@ -275,12 +278,16 @@ func typeToTableHeaders(t reflect.Type, requireDefault bool) ([]string, string,
 
 	headers := []string{}
 	defaultSortName := ""
+	noSortOpt := false
 	for i := 0; i < t.NumField(); i++ {
 		field := t.Field(i)
-		name, defaultSort, recursive, skip, err := parseTableStructTag(field)
+		name, defaultSort, noSort, recursive, skip, err := parseTableStructTag(field)
 		if err != nil {
 			return nil, "", xerrors.Errorf("parse struct tags for field %q in type %q: %w", field.Name, t.String(), err)
 		}
+		if requireDefault && noSort {
+			noSortOpt = true
+		}
 
 		if name == "" && (recursive && skip) {
 			return nil, "", xerrors.Errorf("a name is required for the field %q. "+
@@ -323,8 +330,8 @@ func typeToTableHeaders(t reflect.Type, requireDefault bool) ([]string, string,
 		headers = append(headers, name)
 	}
 
-	if defaultSortName == "" && requireDefault {
-		return nil, "", xerrors.Errorf("no field marked as default_sort in type %q", t.String())
+	if defaultSortName == "" && requireDefault && !noSortOpt {
+		return nil, "", xerrors.Errorf("no field marked as default_sort or nosort in type %q", t.String())
 	}
 
 	return headers, defaultSortName, nil
@@ -351,7 +358,7 @@ func valueToTableMap(val reflect.Value) (map[string]any, error) {
 	for i := 0; i < val.NumField(); i++ {
 		field := val.Type().Field(i)
 		fieldVal := val.Field(i)
-		name, _, recursive, skip, err := parseTableStructTag(field)
+		name, _, _, recursive, skip, err := parseTableStructTag(field)
 		if err != nil {
 			return nil, xerrors.Errorf("parse struct tags for field %q in type %T: %w", field.Name, val, err)
 		}

diff --git a/cli/speedtest.go b/cli/speedtest.go
@@ -18,19 +18,19 @@ import (
 	"github.com/coder/serpent"
 )
 
-type speedtestResult struct {
-	Overall   speedtestResultInterval   `json:"overall"`
-	Intervals []speedtestResultInterval `json:"intervals"`
+type SpeedtestResult struct {
+	Overall   SpeedtestResultInterval   `json:"overall"`
+	Intervals []SpeedtestResultInterval `json:"intervals"`
 }
 
-type speedtestResultInterval struct {
+type SpeedtestResultInterval struct {
 	StartTimeSeconds float64 `json:"start_time_seconds"`
 	EndTimeSeconds   float64 `json:"end_time_seconds"`
 	ThroughputMbits  float64 `json:"throughput_mbits"`
 }
 
 type speedtestTableItem struct {
-	Interval   string `table:"Interval,default_sort"`
+	Interval   string `table:"Interval,nosort"`
 	Throughput string `table:"Throughput"`
 }
 
@@ -42,7 +42,7 @@ func (r *RootCmd) speedtest() *serpent.Command {
 		pcapFile  string
 		formatter = cliui.NewOutputFormatter(
 			cliui.ChangeFormatterData(cliui.TableFormat([]speedtestTableItem{}, []string{"Interval", "Throughput"}), func(data any) (any, error) {
-				res, ok := data.(speedtestResult)
+				res, ok := data.(SpeedtestResult)
 				if !ok {
 					// This should never happen
 					return "", xerrors.Errorf("expected speedtestResult, got %T", data)
@@ -56,7 +56,7 @@ func (r *RootCmd) speedtest() *serpent.Command {
 				}
 				tableRows[len(res.Intervals)] = cliui.TableSeparator{}
 				tableRows[len(res.Intervals)+1] = speedtestTableItem{
-					Interval:   "Total",
+					Interval:   fmt.Sprintf("%.2f-%.2f sec", res.Overall.StartTimeSeconds, res.Overall.EndTimeSeconds),
 					Throughput: fmt.Sprintf("%.4f Mbits/sec", res.Overall.ThroughputMbits),
 				}
 				return tableRows, nil
@@ -167,19 +167,20 @@ func (r *RootCmd) speedtest() *serpent.Command {
 			if err != nil {
 				return err
 			}
-			var outputResult speedtestResult
+			var outputResult SpeedtestResult
 			startTime := results[0].IntervalStart
-			outputResult.Intervals = make([]speedtestResultInterval, len(results)-1)
+			outputResult.Intervals = make([]SpeedtestResultInterval, len(results)-1)
 			for i, r := range results {
-				tmp := speedtestResultInterval{
+				interval := SpeedtestResultInterval{
 					StartTimeSeconds: r.IntervalStart.Sub(startTime).Seconds(),
 					EndTimeSeconds:   r.IntervalEnd.Sub(startTime).Seconds(),
 					ThroughputMbits:  r.MBitsPerSecond(),
 				}
 				if r.Total {
-					outputResult.Overall = tmp
+					interval.StartTimeSeconds = 0
+					outputResult.Overall = interval
 				} else {
-					outputResult.Intervals[i] = tmp
+					outputResult.Intervals[i] = interval
 				}
 			}
 			out, err := formatter.Format(inv.Context(), outputResult)

diff --git a/cli/speedtest_test.go b/cli/speedtest_test.go
@@ -1,7 +1,9 @@
 package cli_test
 
 import (
+	"bytes"
 	"context"
+	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -10,6 +12,7 @@ import (
 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/v2/agent/agenttest"
+	"github.com/coder/coder/v2/cli"
 	"github.com/coder/coder/v2/cli/clitest"
 	"github.com/coder/coder/v2/coderd/coderdtest"
 	"github.com/coder/coder/v2/codersdk"
@@ -56,3 +59,45 @@ func TestSpeedtest(t *testing.T) {
 	})
 	<-cmdDone
 }
+
+func TestSpeedtestJson(t *testing.T) {
+	t.Parallel()
+	t.Skip("Potentially flaky test - see https://github.com/coder/coder/issues/6321")
+	if testing.Short() {
+		t.Skip("This test takes a minimum of 5ms per a hardcoded value in Tailscale!")
+	}
+	client, workspace, agentToken := setupWorkspaceForAgent(t)
+	_ = agenttest.New(t, client.URL, agentToken)
+	coderdtest.AwaitWorkspaceAgents(t, client, workspace.ID)
+
+	ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+	defer cancel()
+
+	require.Eventually(t, func() bool {
+		ws, err := client.Workspace(ctx, workspace.ID)
+		if !assert.NoError(t, err) {
+			return false
+		}
+		a := ws.LatestBuild.Resources[0].Agents[0]
+		return a.Status == codersdk.WorkspaceAgentConnected &&
+			a.LifecycleState == codersdk.WorkspaceAgentLifecycleReady
+	}, testutil.WaitLong, testutil.IntervalFast, "agent is not ready")
+
+	inv, root := clitest.New(t, "speedtest", "--output=json", workspace.Name)
+	clitest.SetupConfig(t, client, root)
+	out := bytes.NewBuffer(nil)
+	inv.Stdout = out
+	ctx, cancel = context.WithTimeout(context.Background(), testutil.WaitLong)
+	defer cancel()
+
+	inv.Logger = slogtest.Make(t, nil).Named("speedtest").Leveled(slog.LevelDebug)
+	cmdDone := tGo(t, func() {
+		err := inv.WithContext(ctx).Run()
+		assert.NoError(t, err)
+	})
+	<-cmdDone
+
+	var result cli.SpeedtestResult
+	require.NoError(t, json.Unmarshal(out.Bytes(), &result))
+	require.Len(t, result.Intervals, 5)
+}
diff --git a/cli/testdata/coder_speedtest_--help.golden b/cli/testdata/coder_speedtest_--help.golden
@@ -6,6 +6,10 @@ USAGE:
   Run upload and download tests from your machine to a workspace
 
 OPTIONS:
+  -c, --column string-array (default: Interval,Throughput)
+          Columns to display in table output. Available columns: Interval,
+          Throughput.
+
   -d, --direct bool
           Specifies whether to wait for a direct connection before testing
           speed.
@@ -14,6 +18,9 @@ OPTIONS:
           Specifies whether to run in reverse mode where the client receives and
           the server sends.
 
+  -o, --output string (default: table)
+          Output format. Available formats: table, json.
+
       --pcap-file string
           Specifies a file to write a network capture to.