Add unit tests for util/util.go#995
Conversation
|
Hi @bromivipo. 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 == nil { | ||
| utiltest.AssertFileContents(t, tt.input, string(tt.content)) | ||
| } |
There was a problem hiding this comment.
can we use here
utiltest.AssertErrorMatch(t, err, tt.wantErr)
utiltest.AssertFileContents(t, tt.input, string(tt.content))
|
|
||
| if err == nil { | ||
| utiltest.AssertFileContents(t, tt.input, tt.content) | ||
| } |
There was a problem hiding this comment.
can we use here AssertErrorMatch, AssertEquals and AssertFileContents without conditions? these functions should handle empty input
There was a problem hiding this comment.
AssertErrorMatch could be used without conditions, but AssertFileContents fails if a file does not exist. So I moved error cases to separate tests.
| os.Exit(1) | ||
| } | ||
| fmt.Fprint(os.Stdout, "success msg") | ||
| os.Exit(0) |
There was a problem hiding this comment.
not sure if this test validates anything useful, probably we don't need it
There was a problem hiding this comment.
it does not validate anything itself. But it is used as a separate process to validate DefaultRunner.
There was a problem hiding this comment.
I agree with Ilia, there is no reason to keep the test that is not clear what it test to
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bromivipo, 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 |
|
New changes are detected. LGTM label has been removed. |
|
/gcbrun |
1 similar comment
|
/gcbrun |
| os.Exit(1) | ||
| } | ||
| fmt.Fprint(os.Stdout, "success msg") | ||
| os.Exit(0) |
There was a problem hiding this comment.
I agree with Ilia, there is no reason to keep the test that is not clear what it test to
| func TestAtomicWrite_Success(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| input string | ||
| content []byte | ||
| mode os.FileMode | ||
| }{ | ||
| { | ||
| name: "valid file path, expect success", | ||
| input: filepath.Join(tmpDir, "test-write-1.txt"), | ||
| content: []byte("test content"), | ||
| mode: 0644, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := AtomicWrite(tt.input, tt.content, tt.mode) | ||
| utiltest.AssertErrorMatch(t, err, nil) | ||
| utiltest.AssertFileContents(t, tt.input, string(tt.content)) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestAtomicWrite_Error(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| input string |
There was a problem hiding this comment.
Please refactor TestAtomicWrite to the single tests, there is no reason to have two table tests with only one case, testing the same function. It is complete against the purpose of the approach.
| } | ||
| } | ||
|
|
||
| func TestAtomicWriteFileStream_Error(t *testing.T) { |
There was a problem hiding this comment.
Same comment as below, please refactor tests to have one test and table testing should cover all success and failure cases
| os.Exit(0) | ||
| } | ||
|
|
||
| func TestDefaultRunnerRun(t *testing.T) { |
There was a problem hiding this comment.
What this test is actually testing?
There was a problem hiding this comment.
It verifies that DefaultRunner.Run captures stdout and stderr from the executed command and propagates execution errors. I've replaced the custom helper process with standard system commands to make it clearer.
Is there anything else you think this test should be checking?
3 participants
This PR contains unit tests for util.go
Initial coverage: 3.3%, current coverage 78.7%