Remove use of run.SetPrepareCmd in tests#2858
Conversation
|
Oh and one other thing I was unsure of the purpose of it was this line when checking browser calls |
mislav
left a comment
There was a problem hiding this 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, "") |
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
This stub used to be git checkout -b feature --no-track origin/feature
| 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], " ")) |
There was a problem hiding this comment.
These used to be stubbed but are no longer present after the rewrite. Can you confirm that this is no longer needed?
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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, "") |
There was a problem hiding this comment.
Should this command exit with nonzero status, since we're emulating not being in a git repo?
You did well. It's a messy workaround, but it compensates for URLs on Windows being escaped differently. Basically, any URL that contains |
|
@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. |
6664234 to
2964895
Compare
|
@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 |
mislav
left a comment
There was a problem hiding this comment.
It looks great. Thank you for the hard work!!
Follow up to #2801 to remove the use of
run.SetPrepareCmdin tests in favor ofrun.Stub.I see that
run.SetPrepareCmdis called fromrun.Stubso I wasn't sure if the intention was to move that to be used privately byrun.Stubafter 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_noResultsForBranchinpkg/cmd/pr/view/view_test.go. Is there an intuitive way to test for the absence of commands withrun.Stub?