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

mafredri · 2022-11-04T12:44:05Z

This PR adds golden files to test cli help output.

Any changes to coder --help or coder server --help now need to be verified via occular output.

Like this:

❯ go test ./cli -run=TestCommandHelp
[...]
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -131,3 +131,3 @@
            	            	                                                    improve our product.
            	            	-                                                   Consumes $CODER_TELEMETRY
            	            	+                                                   Consumes $CODER_TELEMETRY_ENABLE
            	            	       --telemetry-trace                            Whether Opentelemetry traces are sent to
            	            	@@ -170,3 +170,3 @@
            	            	                                                    https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
            	            	-                                                   Consumes $CODER_TRACE
            	            	+                                                   Consumes $CODER_TRACE_ENABLE
            	            	       --trace-honeycomb-api-key string             Enables trace exporting to Honeycomb.io
            	Test:       	TestCommandHelpIsGolden/coder_server_--help
            	Messages:   	golden file mismatch: testdata/coder_server_--help.golden, run "make gen", verify and commit the changes
FAIL
FAIL	github.com/coder/coder/cli	0.159s
FAIL

❯ make gen
❯ git add . -p
diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden
index fb71406a..12031e2c 100644
--- a/cli/testdata/coder_server_--help.golden
+++ b/cli/testdata/coder_server_--help.golden
@@ -129,7 +129,7 @@ Flags:
       --telemetry                                  Whether telemetry is enabled or not. Coder
                                                    collects anonymized usage data to help
                                                    improve our product.
-                                                   Consumes $CODER_TELEMETRY
+                                                   Consumes $CODER_TELEMETRY_ENABLE
       --telemetry-trace                            Whether Opentelemetry traces are sent to
                                                    Coder. Coder collects anonymized
                                                    application tracing to help improve our
(1/2) Stage this hunk [y,n,q,a,d,j,J,g,/,e,?]? y
@@ -168,7 +168,7 @@ Flags:
                                                    collected. It exports to a backend
                                                    configured by environment variables. See:
                                                    https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
-                                                   Consumes $CODER_TRACE
+                                                   Consumes $CODER_TRACE_ENABLE
       --trace-honeycomb-api-key string             Enables trace exporting to Honeycomb.io
                                                    using the provided API Key.
                                                    Consumes $CODER_TRACE_HONEYCOMB_API_KEY
(2/2) Stage this hunk [y,n,q,a,d,K,g,/,e,?]? y

Emyrk

Oh this is an interesting way of doing it. The makefile updates the golden files

Emyrk · 2022-11-04T13:31:41Z

cli/root_test.go

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


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

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.

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?

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?

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.

Makefile

mtojek · 2022-11-04T14:26:07Z

cli/testdata/coder_--help.golden

+Get Started:
+  - Start a Coder server:                                                       
+
+      [;m$ coder server[0m 


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.

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.)

mtojek · 2022-11-04T14:27:20Z

cli/root_test.go

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


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

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).

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

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

This reverts commit 0928b8b.

mafredri force-pushed the mafredri/cli-help-golden-files branch from 6db8c38 to 71ad866 Compare November 4, 2022 12:46

mafredri marked this pull request as ready for review November 4, 2022 12:48

mafredri requested a review from a team November 4, 2022 12:48

mafredri force-pushed the mafredri/cli-help-golden-files branch 6 times, most recently from c652c69 to c814fa3 Compare November 4, 2022 13:10

Emyrk reviewed Nov 4, 2022

View reviewed changes

feat: Add golden files to test cli help output

7f6b0f4

mafredri force-pushed the mafredri/cli-help-golden-files branch from c814fa3 to 7f6b0f4 Compare November 4, 2022 13:30

Emyrk requested changes Nov 4, 2022

View reviewed changes

deansheather reviewed Nov 4, 2022

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Makefile Outdated Show resolved Hide resolved

mafredri requested a review from deansheather November 4, 2022 14:00

Separate make command -> make update-golden-files

1b4dbe9

mafredri force-pushed the mafredri/cli-help-golden-files branch from 3ec3ae3 to 1b4dbe9 Compare November 4, 2022 14:03

deansheather approved these changes Nov 4, 2022

View reviewed changes

mafredri self-assigned this Nov 4, 2022

Fix test on Windows with CRLF lines

6450177

mtojek reviewed Nov 4, 2022

View reviewed changes

mafredri added 3 commits November 4, 2022 16:43

Unset all envs

0928b8b

Revert "Unset all envs"

91936c1

This reverts commit 0928b8b.

Remove CRLF when reading too

41e2660

Emyrk approved these changes Nov 4, 2022

View reviewed changes

mafredri merged commit 587924f into main Nov 4, 2022

mafredri deleted the mafredri/cli-help-golden-files branch November 4, 2022 17:48

github-actions bot locked and limited conversation to collaborators Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

mafredri commented Nov 4, 2022 •

edited

Loading

Uh oh!

Emyrk left a comment

Uh oh!

Emyrk Nov 4, 2022

Uh oh!

mafredri Nov 4, 2022

Uh oh!

Emyrk Nov 4, 2022

Uh oh!

mafredri Nov 4, 2022

Uh oh!

mafredri Nov 4, 2022

Uh oh!

Emyrk Nov 4, 2022

Uh oh!

Uh oh!

Uh oh!

mtojek Nov 4, 2022

Uh oh!

mafredri Nov 4, 2022

Uh oh!

mtojek Nov 4, 2022

Uh oh!

mafredri Nov 4, 2022

Uh oh!

mafredri Nov 4, 2022 •

edited

Loading

Uh oh!

mtojek Nov 4, 2022

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

mafredri commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Emyrk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mafredri commented Nov 4, 2022 •

edited

Loading

mafredri Nov 4, 2022 •

edited

Loading