Skip to content

feat: Add golden files to test cli help output #4897

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Nov 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ gotests.coverage
.gitpod.yml
.DS_Store

# Make target for updating golden files.
cli/testdata/.gen-golden

# Front-end ignore
.next/
site/.eslintcache
Expand Down
9 changes: 9 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,15 @@ site/src/api/typesGenerated.ts: scripts/apitypings/main.go $(shell find codersdk
cd site
yarn run format:types

update-golden-files: cli/testdata/.gen-golden
.PHONY: update-golden-files

cli/testdata/.gen-golden: $(wildcard cli/testdata/*.golden) \
$(shell find . -not -path './vendor/*' -type f -name '*.go')

go test ./cli -run=TestCommandHelp -update
touch "$@"

test: test-clean
gotestsum -- -v -short ./...
.PHONY: test
Expand Down
76 changes: 76 additions & 0 deletions cli/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,12 @@ package cli_test

import (
"bytes"
"flag"
"net/http"
"net/http/httptest"
"os"
"path/filepath"
"strings"
"testing"

"github.com/spf13/cobra"
Expand All @@ -16,8 +20,80 @@ import (
"github.com/coder/coder/cli/cliflag"
"github.com/coder/coder/cli/clitest"
"github.com/coder/coder/codersdk"
"github.com/coder/coder/testutil"
)

var updateGoldenFiles = flag.Bool("update", false, "update .golden files")

//nolint:tparallel,paralleltest // These test sets env vars.
func TestCommandHelp(t *testing.T) {
t.Parallel()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You use t.Setenv in the subtests. Can't we not set envs during parallel tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, t.Setenv will panic after a call to t.Parallel(). It modifies the os env but restores it at the end of the test.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, apparently it does not panic if you use t.Setenv in a subtest and you have t.Parallel on the root test.

TestCommandHelp sets t.Parallel on the first line, but subtests use t.Setenv. Do we need to remove t.Parallel from the root test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh?

This is from the Go implementation:

func (t *T) Setenv(key, value string) {
	if t.isParallel {
		panic("testing: t.Setenv called after t.Parallel; cannot set environment variables in parallel tests")
	}

So if the subtest is parallel, it'll panic, no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I think I misunderstood. It’s fine if the parent test has t.Parallel. Just means that starting it will not block other tests, but the subtests will.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


tests := []struct {
name string
cmd []string
env map[string]string
}{
{
name: "coder --help",
cmd: []string{"--help"},
env: map[string]string{
"CODER_CONFIG_DIR": "/tmp/coder-cli-test-config",
},
},
{
name: "coder server --help",
cmd: []string{"server", "--help"},
env: map[string]string{
"CODER_CONFIG_DIR": "/tmp/coder-cli-test-config",
"CODER_CACHE_DIRECTORY": "/tmp/coder-cli-test-cache",
},
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
// Unset all CODER_ environment variables for a clean slate.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if you shouldn't remove all environment variables, like TTY-related, NO_COLOR, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was being cautious by only overriding the CODER ones, but I'm guessing it won't hurt 👍🏻. Currently (to my knowledge) the only non-coder env that has an effect on the application is CACHE_DIRECTORY (for systemd compat).

Copy link
Member Author

@mafredri mafredri Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, looks like it broke Windows tests, I'll just revert the change and we can fix any issues if they surface in the future. https://github.com/coder/coder/actions/runs/3394713015/jobs/5643623340

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Maybe there is a minimal set of env vars that are required by Windows.

for _, kv := range os.Environ() {
name := strings.Split(kv, "=")[0]
if _, ok := tt.env[name]; !ok && strings.HasPrefix(name, "CODER_") {
t.Setenv(name, "")
}
}
// Override environment variables for a reproducible test.
for k, v := range tt.env {
t.Setenv(k, v)
}

ctx, _ := testutil.Context(t)

var buf bytes.Buffer
root, _ := clitest.New(t, tt.cmd...)
root.SetOut(&buf)
err := root.ExecuteContext(ctx)
require.NoError(t, err)

got := buf.Bytes()
// Remove CRLF newlines (Windows).
got = bytes.ReplaceAll(got, []byte{'\r', '\n'}, []byte{'\n'})

gf := filepath.Join("testdata", strings.Replace(tt.name, " ", "_", -1)+".golden")
if *updateGoldenFiles {
t.Logf("update golden file for: %q: %s", tt.name, gf)
err = os.WriteFile(gf, got, 0o600)
require.NoError(t, err, "update golden file")
}

want, err := os.ReadFile(gf)
require.NoError(t, err, "read golden file, run \"make update-golden-files\" and commit the changes")
// Remove CRLF newlines (Windows).
want = bytes.ReplaceAll(want, []byte{'\r', '\n'}, []byte{'\n'})
require.Equal(t, string(want), string(got), "golden file mismatch: %s, run \"make update-golden-files\", verify and commit the changes", gf)
})
}
}

func TestRoot(t *testing.T) {
t.Parallel()
t.Run("FormatCobraError", func(t *testing.T) {
Expand Down
67 changes: 67 additions & 0 deletions cli/testdata/coder_--help.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
Coder v0.0.0-devel — A tool for provisioning self-hosted development environments with Terraform.

Usage:
coder [flags]

coder [command]

Get Started:
- Start a Coder server:

$ coder server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you may consider disabling colors for readability purposes. I assume that the major goal of this change request is to check the CLI text output.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They should already be since stdout is not a tty, so seems like a separate issue 😅. (You're absolutely right that it shouldn't be output since we only care about the text.)


- Get started by creating a template from an example:

$ coder templates init

Commands:
completion Generate the autocompletion script for the specified shell
dotfiles Checkout and install a dotfiles repository from a Git URL
help Help about any command
login Authenticate with Coder deployment
logout Unauthenticate your local session
port-forward Forward ports from machine to a workspace
publickey Output your Coder public key used for Git operations
reset-password Directly connect to the database to reset a user's password
server Start a Coder server
state Manually manage Terraform state to fix broken workspaces
templates Manage templates
tokens Manage personal access tokens
users Manage users
version Show coder version

Workspace Commands:
config-ssh Add an SSH Host entry for your workspaces "ssh coder.workspace"
create Create a workspace
delete Delete a workspace
list List workspaces
schedule Schedule automated start and stop times for workspaces
show Display details of a workspace's resources and agents
speedtest Run upload and download tests from your machine to a workspace
ssh Start a shell into a workspace
start Start a workspace
stop Stop a workspace
update Update a workspace

Flags:
--experimental Enable experimental features. Experimental features are not
ready for production.
Consumes $CODER_EXPERIMENTAL
--global-config coder Path to the global coder config directory.
Consumes $CODER_CONFIG_DIR (default "/tmp/coder-cli-test-config")
--header stringArray HTTP headers added to all requests. Provide as "Key=Value".
Consumes $CODER_HEADER
-h, --help help for coder
--no-feature-warning Suppress warnings about unlicensed features.
Consumes $CODER_NO_FEATURE_WARNING
--no-version-warning Suppress warning when client and server versions do not match.
Consumes $CODER_NO_VERSION_WARNING
--token string Specify an authentication token. For security reasons setting
CODER_SESSION_TOKEN is preferred.
Consumes $CODER_SESSION_TOKEN
--url string URL to a deployment.
Consumes $CODER_URL
-v, --verbose Enable verbose output.
Consumes $CODER_VERBOSE

Use "coder [command] --help" for more information about a command.
Loading