chore(linters): Enable usetesting linter
#16456
Conversation
logger/structured_logger_test.go
Outdated
| // t.TempDir() doesn't work properly on windows for this case | ||
| //nolint:usetesting // "os.CreateTemp("", ...) could be replaced by os.CreateTemp(t.TempDir(), ...) in TestStructuredFile" | ||
| tmpfile, err := os.CreateTemp("", "") | ||
| require.NoError(t, err) | ||
| defer os.Remove(tmpfile.Name()) |
There was a problem hiding this comment.
Hmm how about
| // t.TempDir() doesn't work properly on windows for this case | |
| //nolint:usetesting // "os.CreateTemp("", ...) could be replaced by os.CreateTemp(t.TempDir(), ...) in TestStructuredFile" | |
| tmpfile, err := os.CreateTemp("", "") | |
| require.NoError(t, err) | |
| defer os.Remove(tmpfile.Name()) | |
| tmpfile := filepath.Join(t.TempDir(), "test.log") |
of course including the changes for the parameters... Same for the instances below.
|
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
| // Deleting open files on Windows returns an error. | ||
| // Therefore, each time a new defaultHandler is created, we ensure that the file previously used for logging is closed. | ||
| // This is because subsequent tests/subtests do not run in a "clean" environment | ||
| // but instead operate on globals set by several init functions within this package. | ||
| func createDefaultHandler(t *testing.T) { | ||
| closeLogger(t) |
There was a problem hiding this comment.
Instead of doing this, couldn't we call defer CloseLogging() in each test?
There was a problem hiding this comment.
Then we would get close /dev/stderr: file already closed during SetupLogging :(
There was a problem hiding this comment.
So how about changing CloseLogging to
func CloseLogging() error {
if instance == nil {
return nil
}
if err := instance.close(); err != nil && !errors.Is(err, os.ErrClosed) {
return err
}
return nil
}I think this would be the correct thing to do as this is what we want anyway. TBH, I don't like the t.Run as it works around independent tests are somehow being dependent...
There was a problem hiding this comment.
So how about changing
CloseLoggingtofunc CloseLogging() error { if instance == nil { return nil } if err := instance.close(); err != nil && !errors.Is(err, os.ErrClosed) { return err } return nil }I think this would be the correct thing to do as this is what we want anyway.
I will try.
TBH, I don't like the
t.Runas it works around independent tests are somehow being dependent...
In this case, it doesn't matter whether it's t.Run(...) or regular tests. This package is structured in such a way that, unfortunately, the tests depend on each other...
|
@srebhan And all the tests are green... |
Summary
In this PR, I'm enabling
usetestinglinter and addressing following findings:Checklist
Related issues
resolves #16412