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.

Sign up for GitHub

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

Jump to bottom

Remove use of run.SetPrepareCmd in tests #2858

Merged

mislav merged 12 commits into cli:trunk from dpromanko:dpromanko/remove-set-cmd-prepare

Jan 28, 2021

Contributor

dpromanko commented Jan 27, 2021

Follow up to #2801 to remove the use of run.SetPrepareCmd in tests in favor of run.Stub.

I see that run.SetPrepareCmd is called from run.Stub so I wasn't sure if the intention was to move that to be used privately by run.Stub after it was no longer being called by tests. With some direction I can do some additional work there, or if a maintainer wants to pick that piece up that is fine as well.

The other thing with this PR I was unsure of is the few places that were testing failure paths and expecting no commands to be executed, for example TestPRView_web_noResultsForBranch in pkg/cmd/pr/view/view_test.go. Is there an intuitive way to test for the absence of commands with run.Stub?

dpromanko added 8 commits

January 26, 2021 08:14


          use Stub instead of SetPrepareCmd in 'issue list' tests

877cbbb


          use Stub instead of SetPrepareCmd in 'pr list' tests

2eed159


          use Stub instead of SetPrepareCmd in 'issue view' tests

3068d80


          use Stub instead of SetPrepareCmd in 'issue create' tests

9dcd3fb


          use Stub instead of SetPrepareCmd in 'pr view' tests

45bc1d7


          use Stub instead of SetPrepareCmd in 'pr checkout' tests

a04e0ec


          use Stub instead of SetPrepareCmd in 'repo create' tests

696cbfc


          remove unused errorStub from 'pr checkout' test

cb897fd

Contributor Author

dpromanko commented Jan 27, 2021

Oh and one other thing I was unsure of the purpose of it was this line when checking browser calls url := strings.ReplaceAll(args[len(args)-1], "^", ""). I copied this usage from here. I'm not sure what case that replace is meant to handle.

mislav suggested changes

View reviewed changes

Contributor

mislav left a comment

This is great. Thanks for addressing this!

Unfortunately, I've spotted a number of translation errors. Do you have time to take another pass over the stubs indicated in my comments?

For asserting that no commands have been ran, simply initialize the stubber as normal and make sure its teardown(t) function gets invoked. That will fail the test if no commands were registered with the stubber, but if gh tried to shell out to something.

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
+              	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")

Contributor

mislav Jan 27, 2021

This stub used to error out with exit code 1 previously.

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
+              	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")
+              	cs.Register(`git checkout feature`, 0, "")

Contributor

mislav Jan 27, 2021

This stub used to be git checkout -b feature --no-track origin/feature

pkg/cmd/pr/checkout/checkout_test.go

Comment on lines -135 to -136

		assert.Equal(t, "git config branch.feature.remote origin", strings.Join(ranCommands[2], " "))
		assert.Equal(t, "git config branch.feature.merge refs/heads/feature", strings.Join(ranCommands[3], " "))

Contributor

mislav Jan 27, 2021

These used to be stubbed but are no longer present after the rewrite. Can you confirm that this is no longer needed?

Contributor Author

dpromanko Jan 28, 2021

No these are needed. This was a mistake on my part of changing test behavior by missing the non-zero exit code. This has been corrected.

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
+              	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")

Contributor

mislav Jan 27, 2021

The previous stub used to fail with exit status 1

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git fetch robot-fork \+refs/heads/feature:refs/remotes/robot-fork/feature`, 0, "")
+              	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")

Contributor

mislav Jan 27, 2021

This stub used to fail with exit status 1

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
+              	cs.Register(`git config branch\.feature\.merge`, 0, "")

Contributor

mislav Jan 27, 2021

This stub used to print the following contents as output: refs/heads/feature\n

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git fetch origin refs/pull/123/head`, 0, "")
+              	cs.Register(`git config branch\.feature\.merge`, 0, "")

Contributor

mislav Jan 27, 2021

This stub used to print the following contents as output: refs/heads/feature\n

pkg/cmd/pr/checkout/checkout_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
+              	cs.Register(`git config branch\.feature\.merge`, 0, "")

Contributor

mislav Jan 27, 2021

This used to exit with error status 1

pkg/cmd/pr/view/view_test.go Outdated

+              	cs, cmdTeardown := run.Stub()
+              	defer cmdTeardown(t)
+              	cs.Register(``, 0, "")

Contributor

mislav Jan 27, 2021

An empty pattern will match anything. Please provide something about the command as pattern. Maybe we should have Register() panic if an empty pattern was passed

Contributor Author

dpromanko Jan 28, 2021

This has been corrected. I did not mean to leave that empty pattern in there. It seems reasonable to have Register() panic if an empty pattern is passed to enforce test quality. Did you want me to make that change in this PR as well? Or would you like a separate PR for that?

pkg/cmd/repo/create/create_test.go Outdated

+              	defer cmdTeardown(t)
+              	cs.Register(`git remote add -f origin https://github\.com/OWNER/REPO\.git`, 0, "")
+              	cs.Register(`git rev-parse --show-toplevel`, 0, "")

Contributor

mislav Jan 27, 2021

Should this command exit with nonzero status, since we're emulating not being in a git repo?

Contributor

mislav commented Jan 27, 2021

Oh and one other thing I was unsure of the purpose of it was this line when checking browser calls url := strings.ReplaceAll(args[len(args)-1], "^", "").

You did well. It's a messy workaround, but it compensates for URLs on Windows being escaped differently. Basically, any URL that contains & in the query string needs to have the ReplaceAll line, otherwise the test will fail on Windows. You are free to clean it up from assertions where the URL will never contain a &, but you can also leave it in. We will probably develop a more idiomatic approach to asserting that the browser has been invoked.

vilmibm added this to Backlog 🗒 in The GitHub CLI via automation

vilmibm moved this from Backlog 🗒 to Needs review 🤔 in The GitHub CLI

Contributor Author

dpromanko commented Jan 27, 2021

@mislav thank you for the thorough review! I'll take another pass through this and should hopefully have it cleaned up by the end of the week.


          fix test behavior changes from migration to run.Stub

dpromanko force-pushed the dpromanko/remove-set-cmd-prepare branch from 6664234 to 2964895 Compare

January 27, 2021 23:46

Contributor Author

dpromanko commented Jan 28, 2021

@mislav I think I have it all cleaned up now. Sorry for the sloppy mistakes changing test behavior, I got a little eager with some copy/paste in those scenarios where it was actually stubbing non-zero exit codes and responses.

The only two tests that still seem slightly off are TestPRCheckout_urlArg_differentBase and TestPRCheckout_branchArg. Both of these tests previously had stubbed non-zero exit code for git show-ref --verify -- refs/heads/feature, however I do not think this was needed. I pulled that case out of the old test implementation locally and the tests still pass, so I think that may have been copy/pasted before but not actually needed.

dpromanko requested a review from mislav

January 28, 2021 00:00

mislav added 3 commits

January 28, 2021 21:58


          Update some stubs to be closer to how git behaves

88c2793


          Unpublish SetPrepareCmd

d86cfe4


          Merge remote-tracking branch 'origin' into dpromanko/remove-set-cmd-p…

051fbbc

…repare

mislav approved these changes

View reviewed changes

Contributor

mislav left a comment

It looks great. Thank you for the hard work!!

The GitHub CLI automation moved this from Needs review 🤔 to Needs to be merged 🎉

mislav merged commit d771bef into cli:trunk

The GitHub CLI automation moved this from Needs to be merged 🎉 to Pending Release 🥚

dpromanko deleted the dpromanko/remove-set-cmd-prepare branch

January 28, 2021 22:03

github-actions bot moved this from Pending Release 🥚 to Done 💤 in The GitHub CLI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment