Minor refactoring of acceptance_test.go: print warnings on closing errors #2545
Conversation
anton-107
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
anton-107
changed the title
anton-107
changed the title
| w.Close() | ||
| err := w.Close() | ||
| if err != nil { | ||
| t.Logf("Warning: failed to close pipe writer: %v", err) |
There was a problem hiding this comment.
was this caused by a real issue you saw?
| prepares := []string{} | ||
| cleanups := []string{} | ||
| var prepares []string | ||
| var cleanups []string |
There was a problem hiding this comment.
Unless we have agreed to one style over whole codebase let's avoid this type of changes.
| @@ -542,7 +542,7 @@ func doComparison(t *testing.T, repls testdiff.ReplacementsContext, dirRef, dirN | |||
| } | |||
|
|
|||
| // The test produced an unexpected output file. | |||
| if !okRef && okNew { | |||
There was a problem hiding this comment.
Why was this changed? This deserves its own PR with explanation.
There was a problem hiding this comment.
In this statement, if okRef is false, then okNew is always true due to a previous if !okRef && !okNew { return } statement in this method. Removing the second condition here makes it easier to comprehend this check. If you agree this is valuable, i will open a separate PR.
There was a problem hiding this comment.
I think it's easier to read (and more resilient to refactorings) if you don't need to know all the code before it.
|
This PR has not received an update in a while. If you want to keep this PR open, please leave a comment below or push a new commit and auto-close will be canceled. |
Changes
VerboseTestWhy
2-4. These changes suggest slightly more idiomatic source code
Tests
existing tests