Add unit tests for policies/recipes/steps.go PART 2#975
Conversation
|
Hi @ganochenkodg. 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. |
| if err := tw.Close(); err != nil { | ||
| t.Fatalf("failed to close tar writer: %v", err) | ||
| } | ||
| return &buf |
There was a problem hiding this comment.
can this be reused in other tests? if so, could you move this to common test utils
There was a problem hiding this comment.
Do you mean whole createTarArchive func? it's a setup func for one specific test. If you talk about one if err != nil { } block, it's too simple and short imo to create a helper
There was a problem hiding this comment.
I'm talking about the whole func. If we don't need something like this in other tests, lets keep as it is
| func Test_checkForConflicts(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
| existingFile := "exists.txt" | ||
| os.WriteFile(filepath.Join(tmpDir, existingFile), []byte("content"), 0644) |
There was a problem hiding this comment.
utiltest has a method WriteToTempFileMust, can you check if that can be reused here?
There was a problem hiding this comment.
yes, applied suggested fix
| } | ||
|
|
||
| // Test_checkForConflicts verifies that archive extraction does not overwrite existing files. | ||
| func Test_checkForConflicts(t *testing.T) { |
There was a problem hiding this comment.
Lets stick to pascal case
There was a problem hiding this comment.
Same comment, it is a common pattern to name test for private functions this way
|
/gcbrun |
| } | ||
|
|
||
| // Test_checkForConflicts verifies that archive extraction does not overwrite existing files. | ||
| func Test_checkForConflicts(t *testing.T) { |
There was a problem hiding this comment.
Same comment, it is a common pattern to name test for private functions this way
|
|
||
| // Test_checkForConflicts verifies that archive extraction does not overwrite existing files. | ||
| func Test_checkForConflicts(t *testing.T) { | ||
| existingFile := "exists.txt" |
There was a problem hiding this comment.
Inline this variable as it used only in one place.
|
/gcbrun |
|
/gcbrun |
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: burov, ganochenkodg, iliatsuprik, petercieslak The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
New changes are detected. LGTM label has been removed. |
|
/gcbrun |
|
/ok-to-test |
|
/test ? |
|
@petercieslak: The following commands are available to trigger required jobs:
Use DetailsIn response to this:
Instructions 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. |
google-oss-prow
Bot
merged commit c0faeb2
into
GoogleCloudPlatform:master
4 participants
Second part of unit tests for steps.go:
github.com/GoogleCloudPlatform/osconfig/policies/recipes/steps.go:286: checkForConflicts 65.0% -> 80.0%
github.com/GoogleCloudPlatform/osconfig/policies/recipes/steps.go:268: decompress 28.6% -> 100.0%