Cover google_update.go by unit tests#941
Cover google_update.go by unit tests#941iliatsuprik wants to merge 2 commits intoGoogleCloudPlatform:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iliatsuprik The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @iliatsuprik. Thanks for your PR. I'm waiting for a GoogleCloudPlatform member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/gcbrun |
| mockCtrl := gomock.NewController(t) | ||
| defer mockCtrl.Finish() | ||
|
|
||
| mockCommandRunner := utilmocks.NewMockCommandRunner(mockCtrl) | ||
| packages.SetCommandRunner(mockCommandRunner) |
There was a problem hiding this comment.
Do we really need to recreate mock each time or we can do it just once?
| updatesCmd := utilmocks.EqCmd(exec.Command(googet, "update")) | ||
| updatesCall := mockCommandRunner.EXPECT(). | ||
| Run(ctx, updatesCmd). | ||
| Return(tt.updatesOut, []byte("stderr"), tt.updatesErr). | ||
| Times(1) | ||
|
|
||
| if tt.expectInstall { | ||
| installArgs := append([]string{"-noconfirm", "install"}, tt.installPkgNames...) | ||
| installCmd := utilmocks.EqCmd(exec.Command(googet, installArgs...)) | ||
| mockCommandRunner.EXPECT(). | ||
| Run(ctx, installCmd). | ||
| After(updatesCall). | ||
| Return(tt.installOut, []byte("stderr"), tt.installErr). | ||
| Times(1) |
There was a problem hiding this comment.
I think declarative way of expected command improve readability of the code and reduce cognitive load of the actual test case, see https://github.com/GoogleCloudPlatform/osconfig/blob/master/packages/pip_test.go#L25
| wantErr error | ||
| }{ | ||
| { | ||
| desc: "success", |
There was a problem hiding this comment.
Please be more specific in name of the test, list what is expected result of the test, success is to generic from my perspective, i suggest to have something like "want install command list packages foo and bar"
There was a problem hiding this comment.
Same on other cases "updates error" -> "googet update error is propogated"
|
|
||
| func TestRunGooGetUpdate(t *testing.T) { | ||
| ctx := context.Background() | ||
| googet := filepath.Join(os.Getenv("GooGetRoot"), "googet.exe") |
There was a problem hiding this comment.
Can we avoid reliance on the environment variable here ?
|
/gcbrun |
2 participants
Coverage increased from 0 to 100%