diff --git a/cli/cliui/output.go b/cli/cliui/output.go index 9f06d0ba5d2cb..d15d18b63fe18 100644 --- 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 DisplayTable(data, f.sort, f.columns) + 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 index 9962678be902a..f1fb8075133c8 100644 --- a/cli/cliui/table.go +++ b/cli/cliui/table.go @@ -22,6 +22,13 @@ func Table() table.Writer { return tableWriter } +// This type can be supplied as part of a slice to DisplayTable +// or to a `TableFormat` `Format` call to render a separator. +// Leading separators are not supported and trailing separators +// are ignored by the table formatter. +// e.g. `[]any{someRow, TableSeparator, someRow}` +type TableSeparator struct{} + // filterTableColumns returns configurations to hide columns // that are not provided in the array. If the array is empty, // no filtering will occur! @@ -47,8 +54,12 @@ func filterTableColumns(header table.Row, columns []string) []table.ColumnConfig return columnConfigs } -// DisplayTable renders a table as a string. The input argument must be a slice -// of structs. At least one field in the struct must have a `table:""` tag +// 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 TableSeparator. +// +// At least one field in the struct must have a `table:""` tag // containing the name of the column in the outputted table. // // If `sort` is not specified, the field with the `table:"$NAME,default_sort"` @@ -66,11 +77,20 @@ 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.New("DisplayTable called with empty interface slice") + } + tableType = reflect.Indirect(reflect.ValueOf(v.Index(0).Interface())).Type() + } else { + tableType = v.Type().Elem() } // Get the list of table column headers. - headersRaw, defaultSort, err := typeToTableHeaders(v.Type().Elem(), true) + headersRaw, defaultSort, err := typeToTableHeaders(tableType, true) if err != nil { return "", xerrors.Errorf("get table headers recursively for type %q: %w", v.Type().Elem().String(), err) } @@ -82,9 +102,8 @@ func DisplayTable(out any, sort string, filterColumns []string) (string, error) } headers := make(table.Row, len(headersRaw)) for i, header := range headersRaw { - headers[i] = header + 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)) @@ -130,6 +149,11 @@ 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, headers, filterColumns) +} + +func renderTable(out any, sort string, headers table.Row, filterColumns []string) (string, error) { + v := reflect.Indirect(reflect.ValueOf(out)) // Setup the table formatter. tw := Table() @@ -143,15 +167,22 @@ func DisplayTable(out any, sort string, filterColumns []string) (string, error) // Write each struct to the table. for i := 0; i < v.Len(); i++ { + cur := v.Index(i).Interface() + _, ok := cur.(TableSeparator) + if ok { + tw.AppendSeparator() + continue + } // Format the row as a slice. - rowMap, err := valueToTableMap(v.Index(i)) + // ValueToTableMap does what `reflect.Indirect` does + rowMap, err := valueToTableMap(reflect.ValueOf(cur)) if err != nil { return "", xerrors.Errorf("get table row map %v: %w", i, err) } 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 } @@ -188,25 +219,28 @@ func DisplayTable(out any, sort string, filterColumns []string) (string, error) // 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": @@ -216,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 { @@ -244,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. "+ @@ -292,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 @@ -320,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/cliui/table_test.go b/cli/cliui/table_test.go index bb0b6c658fe45..5772c5cf5869e 100644 --- a/cli/cliui/table_test.go +++ b/cli/cliui/table_test.go @@ -218,6 +218,42 @@ Alice 25 compareTables(t, expected, out) }) + // This test ensures we can display dynamically typed slices + t.Run("Interfaces", func(t *testing.T) { + t.Parallel() + + in := []any{tableTest1{}} + out, err := cliui.DisplayTable(in, "", nil) + t.Log("rendered table:\n" + out) + require.NoError(t, err) + other := []tableTest1{{}} + expected, err := cliui.DisplayTable(other, "", nil) + require.NoError(t, err) + compareTables(t, expected, out) + }) + + t.Run("WithSeparator", func(t *testing.T) { + t.Parallel() + expected := ` +NAME AGE ROLES SUB 1 NAME SUB 1 AGE SUB 2 NAME SUB 2 AGE SUB 3 INNER NAME SUB 3 INNER AGE SUB 4 TIME TIME PTR +bar 20 [a] bar1 21 bar3 23 {bar4 24 } 2022-08-02T15:49:10Z +------------------------------------------------------------------------------------------------------------------------------------------------------------- +baz 30 [] baz1 31 baz3 33 {baz4 34 } 2022-08-02T15:49:10Z +------------------------------------------------------------------------------------------------------------------------------------------------------------- +foo 10 [a b c] foo1 11 foo2 12 foo3 13 {foo4 14 } 2022-08-02T15:49:10Z 2022-08-02T15:49:10Z + ` + + var inlineIn []any + for _, v := range in { + inlineIn = append(inlineIn, v) + inlineIn = append(inlineIn, cliui.TableSeparator{}) + } + out, err := cliui.DisplayTable(inlineIn, "", nil) + t.Log("rendered table:\n" + out) + require.NoError(t, err) + compareTables(t, expected, out) + }) + // This test ensures that safeties against invalid use of `table` tags // causes errors (even without data). t.Run("Errors", func(t *testing.T) { @@ -255,14 +291,6 @@ Alice 25 _, err := cliui.DisplayTable(in, "", nil) require.Error(t, err) }) - - t.Run("WithData", func(t *testing.T) { - t.Parallel() - - in := []any{tableTest1{}} - _, err := cliui.DisplayTable(in, "", nil) - require.Error(t, err) - }) }) t.Run("NotStruct", func(t *testing.T) { diff --git a/cli/speedtest.go b/cli/speedtest.go index 9f8090ef99731..3e0e74668b3b7 100644 --- a/cli/speedtest.go +++ b/cli/speedtest.go @@ -6,7 +6,6 @@ import ( "os" "time" - "github.com/jedib0t/go-pretty/v6/table" "golang.org/x/xerrors" tsspeedtest "tailscale.com/net/speedtest" "tailscale.com/wgengine/capture" @@ -19,12 +18,51 @@ import ( "github.com/coder/serpent" ) +type SpeedtestResult struct { + Overall SpeedtestResultInterval `json:"overall"` + Intervals []SpeedtestResultInterval `json:"intervals"` +} + +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,nosort"` + Throughput string `table:"Throughput"` +} + func (r *RootCmd) speedtest() *serpent.Command { var ( direct bool duration time.Duration direction string pcapFile string + formatter = cliui.NewOutputFormatter( + cliui.ChangeFormatterData(cliui.TableFormat([]speedtestTableItem{}, []string{"Interval", "Throughput"}), func(data any) (any, error) { + res, ok := data.(SpeedtestResult) + if !ok { + // This should never happen + return "", xerrors.Errorf("expected speedtestResult, got %T", data) + } + tableRows := make([]any, len(res.Intervals)+2) + for i, r := range res.Intervals { + tableRows[i] = speedtestTableItem{ + Interval: fmt.Sprintf("%.2f-%.2f sec", r.StartTimeSeconds, r.EndTimeSeconds), + Throughput: fmt.Sprintf("%.4f Mbits/sec", r.ThroughputMbits), + } + } + tableRows[len(res.Intervals)] = cliui.TableSeparator{} + tableRows[len(res.Intervals)+1] = speedtestTableItem{ + Interval: fmt.Sprintf("%.2f-%.2f sec", res.Overall.StartTimeSeconds, res.Overall.EndTimeSeconds), + Throughput: fmt.Sprintf("%.4f Mbits/sec", res.Overall.ThroughputMbits), + } + return tableRows, nil + }), + cliui.JSONFormat(), + ) ) client := new(codersdk.Client) cmd := &serpent.Command{ @@ -124,24 +162,32 @@ func (r *RootCmd) speedtest() *serpent.Command { default: return xerrors.Errorf("invalid direction: %q", direction) } - cliui.Infof(inv.Stdout, "Starting a %ds %s test...", int(duration.Seconds()), tsDir) + cliui.Infof(inv.Stderr, "Starting a %ds %s test...", int(duration.Seconds()), tsDir) results, err := conn.Speedtest(ctx, tsDir, duration) if err != nil { return err } - tableWriter := cliui.Table() - tableWriter.AppendHeader(table.Row{"Interval", "Throughput"}) + var outputResult SpeedtestResult startTime := results[0].IntervalStart - for _, r := range results { + outputResult.Intervals = make([]SpeedtestResultInterval, len(results)-1) + for i, r := range results { + interval := SpeedtestResultInterval{ + StartTimeSeconds: r.IntervalStart.Sub(startTime).Seconds(), + EndTimeSeconds: r.IntervalEnd.Sub(startTime).Seconds(), + ThroughputMbits: r.MBitsPerSecond(), + } if r.Total { - tableWriter.AppendSeparator() + interval.StartTimeSeconds = 0 + outputResult.Overall = interval + } else { + outputResult.Intervals[i] = interval } - tableWriter.AppendRow(table.Row{ - fmt.Sprintf("%.2f-%.2f sec", r.IntervalStart.Sub(startTime).Seconds(), r.IntervalEnd.Sub(startTime).Seconds()), - fmt.Sprintf("%.4f Mbits/sec", r.MBitsPerSecond()), - }) } - _, err = fmt.Fprintln(inv.Stdout, tableWriter.Render()) + out, err := formatter.Format(inv.Context(), outputResult) + if err != nil { + return err + } + _, err = fmt.Fprintln(inv.Stdout, out) return err }, } @@ -173,5 +219,6 @@ func (r *RootCmd) speedtest() *serpent.Command { Value: serpent.StringOf(&pcapFile), }, } + formatter.AttachOptions(&cmd.Options) return cmd } diff --git a/cli/speedtest_test.go b/cli/speedtest_test.go index 9878ff04ab527..281fdcc1488d0 100644 --- 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 index 60eb4026b1028..538c955fae252 100644 --- 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. diff --git a/docs/cli/speedtest.md b/docs/cli/speedtest.md index e2d3a435fb0ea..ab9d9a4f7e49c 100644 --- a/docs/cli/speedtest.md +++ b/docs/cli/speedtest.md @@ -45,3 +45,21 @@ Specifies the duration to monitor traffic. | Type | string | Specifies a file to write a network capture to. + +### -c, --column + +| | | +| ------- | -------------------------------- | +| Type | string-array | +| Default | Interval,Throughput | + +Columns to display in table output. Available columns: Interval, Throughput. + +### -o, --output + +| | | +| ------- | ------------------- | +| Type | string | +| Default | table | + +Output format. Available formats: table, json.