chore: use require instead of t.Fatal (part 2) #2857
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
lifecycle_test.go
Outdated
| @@ -919,7 +919,7 @@ func TestPrintContainerLogsOnError(t *testing.T) { | |||
| break | |||
| } | |||
|
|
|||
| t.Fatal("Read Error:", err) | |||
| require.NoErrorf(t, err, "Read Error") | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I knew, maybe you prefer it the now ?
| @@ -108,21 +106,19 @@ func TestElasticsearch(t *testing.T) { | |||
|
|
|||
| if tt.image != baseImage8 && err != nil { | |||
There was a problem hiding this comment.
We probably need to restructure this if/else sequence here, as it was not already very readable before the changes, and the readability does not improve after them.
07d1b2d to
9dfd622
Compare
logconsumer_test.go
Outdated
| if err != nil || i > 0 { | ||
| t.Fatalf("Can't make request to nginx container from dind container") | ||
| } | ||
| require.Zerof(t, i, "Can't make request to nginx container from dind container") |
There was a problem hiding this comment.
Comment NOT related to this PR, but for the general maintenance of the library:
@stevenh wdyt of keeping the description messages in the checks? Should we keep them, or remove them. We are working in creating more and more contextual error messages, so at some point we'll have much better experience debugging test errors.
There was a problem hiding this comment.
Personally I don't think they add value in most cases, just adding more for the developer to read. This is particularly the case for error checks.
The way I look at it is does it make it clearer to the reader why this might have failed, which this type of message I would argue doesn't as its obvious from the line above that the request didn't work.
In this particular example they checks are back to front, error check should come first.
| if resp.StatusCode != http.StatusOK { | ||
| tt.Fatalf("unexpected status code: %d", resp.StatusCode) | ||
| } | ||
| require.Equalf(tt, http.StatusOK, resp.StatusCode, "unexpected status code: %d", resp.StatusCode) |
There was a problem hiding this comment.
This is an example of a message that people might think is helpful but isn't in my mind as the values are output anyway and is obvious from the code its a status check.
| defer resp.Body.Close() | ||
| require.NoError(t, err) | ||
|
|
||
| if tt.image == baseImage8 && tt.passwordCustomiser == nil { |
There was a problem hiding this comment.
suggestion: This would be easier to follow if we changed to switch on the image and had each case inside.
There was a problem hiding this comment.
No need to apply it, I did it it for the next condition
d737429 to
100be4d
Compare
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
100be4d to
edbfde9
Compare
There was a problem hiding this comment.
LGTM!
As a follow-up, I'd send a separate PR replacing the formatted requires, simply removing the messages, as it's now much clearer the reasons why they failed.
Thank you @mmorel-35 for your hard work cleaning up the project 🙇
What does this PR do?
This uses
requireinstead oft.FatalcallsWhy is it important?
This reduce the number of lines required to make an assertion.
It also make the assertion easier to understand.
Related issues