Removes test of default editor passthrough from git #1080

fhats-stripe · 2023-05-24T22:04:54Z

Reviewers

r? @charliecruzan-stripe
cc @stripe/developer-products

Summary

I set up the stripe CLI for the first time on my laptop today and ran make test as suggested by the README. Everything passed except for one test, which tests that when an editor preference is not given to git that it uses a preference from the current environment (the EDITOR variable). I keep EDITOR set to vim on my system, which causes the test to fail as it wants vi explicitly.

My gut feeling is that this test isn't really covering functionality that's unique to the Stripe CLI, and is instead covering how git's editor defaults are computed. I thought about setting EDITOR in the test's setup, which would have to change based on runtime.GOOS. Then I figured that we might just be better off without the test at all, if all we're doing is checking that git grabs its editor default from the environment? 🤷

Anyway, happy to reverse course on this and keep the test with some setup to make it more durable when EDITOR is set if we'd prefer to keep it! Just let me know :)

Testing

Originally fails in the following manner on my system:

fhats@st-fhats2:~/stripe/stripe-cli$make test                                                                                                                                                                                                                   master 14:07:46
go test  -failfast -race -coverpkg=./... -covermode=atomic -coverprofile=coverage.txt ./... -run . -timeout=2m
?   	github.com/stripe/stripe-cli/cmd/stripe	[no test files]
?   	github.com/stripe/stripe-cli/pkg/ansi	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/cmd	2.378s	coverage: 17.8% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/cmd/logs	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/cmd/plugin	1.398s	coverage: 15.9% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/cmd/resource	0.776s	coverage: 18.1% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/cmd/samples	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/config	1.569s	coverage: 16.7% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/fixtures	2.209s	coverage: 20.2% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/gen	1.183s	coverage: 16.1% of statements in ./...
--- FAIL: TestGetDefaultGitEditor (0.19s)
    --- FAIL: TestGetDefaultGitEditor/no_GIT_EDITOR_or_EDITOR_defaults_to_vi (0.06s)
        editor_test.go:101:
            	Error Trace:	/Users/fhats/stripe/stripe-cli/pkg/git/editor_test.go:101
            	Error:      	Not equal:
            	            	expected: "vi"
            	            	actual  : "vim"

            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1 @@
            	            	-vi
            	            	+vim
            	Test:       	TestGetDefaultGitEditor/no_GIT_EDITOR_or_EDITOR_defaults_to_vi
FAIL
coverage: 16.2% of statements in ./...
FAIL	github.com/stripe/stripe-cli/pkg/git	2.016s
ok  	github.com/stripe/stripe-cli/pkg/login	0.440s	coverage: 18.9% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login/acct	0.597s	coverage: 16.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login/keys	2.047s	coverage: 17.8% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/logout	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/logtailing	0.705s	coverage: 15.9% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/open	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/parsers	0.611s	coverage: 17.4% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/plugins	0.771s	coverage: 20.4% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/plugins/proto	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/proxy	0.549s	coverage: 17.7% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/requests	0.549s	coverage: 19.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/rpcservice	0.557s	coverage: 30.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/samples	0.503s	coverage: 16.6% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/spec	3.582s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/status	0.550s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/stripe	0.772s	coverage: 17.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/stripeauth	0.615s	coverage: 16.7% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/terminal	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/terminal/p400	0.681s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/useragent	0.799s	coverage: 15.8% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/validators	0.638s	coverage: 16.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/version	0.742s	coverage: 15.8% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/websocket	0.750s	coverage: 17.5% of statements in ./...
?   	github.com/stripe/stripe-cli/rpc	[no test files]
FAIL
make: *** [test] Error 1

and it's reproducable based on whatever you set EDITOR to (like /bin/shuf, a nefarious trick):

fhats@st-fhats2:~/stripe/stripe-cli$EDITOR="/bin/shuf" make test                                                                                                                                                                                 fhats/fix-editor-test 14:22:32
go test  -failfast -race -coverpkg=./... -covermode=atomic -coverprofile=coverage.txt ./... -run . -timeout=2m
?   	github.com/stripe/stripe-cli/cmd/stripe	[no test files]
?   	github.com/stripe/stripe-cli/pkg/ansi	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/cmd	2.291s	coverage: 17.8% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/cmd/logs	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/cmd/plugin	0.759s	coverage: 15.9% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/cmd/resource	0.906s	coverage: 18.1% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/cmd/samples	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/config	1.164s	coverage: 16.7% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/fixtures	1.548s	coverage: 20.2% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/gen	1.355s	coverage: 16.1% of statements in ./...
--- FAIL: TestGetDefaultGitEditor (0.12s)
    --- FAIL: TestGetDefaultGitEditor/no_GIT_EDITOR_or_EDITOR_defaults_to_vi (0.03s)
        editor_test.go:101:
            	Error Trace:	/Users/fhats/stripe/stripe-cli/pkg/git/editor_test.go:101
            	Error:      	Not equal:
            	            	expected: "vi"
            	            	actual  : "/bin/shuf"

            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1 @@
            	            	-vi
            	            	+/bin/shuf
            	Test:       	TestGetDefaultGitEditor/no_GIT_EDITOR_or_EDITOR_defaults_to_vi
FAIL
coverage: 16.2% of statements in ./...
FAIL	github.com/stripe/stripe-cli/pkg/git	0.744s
ok  	github.com/stripe/stripe-cli/pkg/login	0.458s	coverage: 18.9% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login/acct	1.847s	coverage: 16.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login/keys	2.139s	coverage: 17.8% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/logout	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/logtailing	0.505s	coverage: 15.9% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/open	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/parsers	0.528s	coverage: 17.4% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/plugins	0.729s	coverage: 20.4% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/plugins/proto	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/proxy	0.540s	coverage: 17.7% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/requests	0.489s	coverage: 19.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/rpcservice	0.739s	coverage: 30.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/samples	0.591s	coverage: 16.6% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/spec	3.692s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/status	0.435s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/stripe	0.574s	coverage: 17.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/stripeauth	0.558s	coverage: 16.7% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/terminal	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/terminal/p400	0.406s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/useragent	0.457s	coverage: 15.8% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/validators	0.527s	coverage: 16.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/version	0.502s	coverage: 15.8% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/websocket	0.475s	coverage: 17.5% of statements in ./...
?   	github.com/stripe/stripe-cli/rpc	[no test files]
FAIL
make: *** [test] Error 1

After this change tests now pass for me locally, even when EDITOR is set to something else:

fhats@st-fhats2:~/stripe/stripe-cli$EDITOR="/bin/shuf" make test                                                                                                                                                                                 fhats/fix-editor-test 14:55:31
go test  -failfast -race -coverpkg=./... -covermode=atomic -coverprofile=coverage.txt ./... -run . -timeout=2m
?   	github.com/stripe/stripe-cli/cmd/stripe	[no test files]
?   	github.com/stripe/stripe-cli/pkg/ansi	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/cmd	2.400s	coverage: 17.8% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/cmd/logs	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/cmd/plugin	0.597s	coverage: 15.9% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/cmd/resource	1.022s	coverage: 18.1% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/cmd/samples	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/config	0.850s	coverage: 16.7% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/fixtures	1.603s	coverage: 20.2% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/gen	1.982s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/git	1.751s	coverage: 16.2% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login	1.814s	coverage: 18.9% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login/acct	0.431s	coverage: 16.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/login/keys	2.272s	coverage: 17.8% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/logout	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/logtailing	0.623s	coverage: 15.9% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/open	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/parsers	0.609s	coverage: 17.4% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/plugins	0.712s	coverage: 20.4% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/plugins/proto	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/proxy	0.658s	coverage: 17.7% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/requests	0.639s	coverage: 19.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/rpcservice	1.069s	coverage: 30.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/samples	0.622s	coverage: 16.6% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/spec	3.906s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/status	0.579s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/stripe	0.625s	coverage: 17.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/stripeauth	0.626s	coverage: 16.7% of statements in ./...
?   	github.com/stripe/stripe-cli/pkg/terminal	[no test files]
ok  	github.com/stripe/stripe-cli/pkg/terminal/p400	0.583s	coverage: 16.1% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/useragent	0.543s	coverage: 15.8% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/validators	0.497s	coverage: 16.5% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/version	0.396s	coverage: 15.8% of statements in ./...
ok  	github.com/stripe/stripe-cli/pkg/websocket	0.450s	coverage: 17.5% of statements in ./...
?   	github.com/stripe/stripe-cli/rpc	[no test files]

pkg/git/editor_test.go

This reverts commit ce1f8b8.

charliecruzan-stripe

nice change 🎉

removes test of default editor passthrough from git

ce1f8b8

fhats-stripe requested a review from a team as a code owner May 24, 2023 22:04

charliecruzan-stripe reviewed May 24, 2023

View reviewed changes

pkg/git/editor_test.go Show resolved Hide resolved

fhats-stripe added 3 commits May 24, 2023 16:07

Puts the test I removed back

c92bf22

This reverts commit ce1f8b8.

makes git editor fallback test aware of EDITOR environment variable

aa05c1d

fixes a dumb error in logic

9f8a1c0

charliecruzan-stripe approved these changes May 25, 2023

View reviewed changes

Merge branch 'master' into fhats/fix-editor-test

d0174e7

fhats-stripe merged commit 609c0f5 into master May 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes test of default editor passthrough from git #1080

Removes test of default editor passthrough from git #1080

fhats-stripe commented May 24, 2023

charliecruzan-stripe left a comment

Removes test of default editor passthrough from git #1080

Removes test of default editor passthrough from git #1080

Conversation

fhats-stripe commented May 24, 2023

Reviewers

Summary

Testing

charliecruzan-stripe left a comment

Choose a reason for hiding this comment