test: use T.TempDir to create temporary test directory

[Juneezee]

What this PR does:
A testing enhancement.

We can use the T.TempDir function from the testing package to create temporary directory. The directory created by T.TempDir is automatically removed when the test and all its subtests complete.

Reference: https://pkg.go.dev/testing#T.TempDir

Which issue(s) this PR fixes:

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[mdisibio]

Hi, thanks for the contribution. We have used t.TempDir in the past but it led to some flaky tests. There is a slight difference in behavior between it and os.RemoveAll in that t.TempDir errors if any new files show up after the test returns, where os.RemoveAll still blindly deletes. Some of our modules have a messy shutdown and a few goroutines may continue writing files after the test returns.

Did you encounter this too, or have any thoughts?

[Juneezee]

Hi, thanks for the contribution. We have used t.TempDir in the past but it led to some flaky tests. There is a slight difference in behavior between it and os.RemoveAll in that t.TempDir errors if any new files show up after the test returns, where os.RemoveAll still blindly deletes. Some of our modules have a messy shutdown and a few goroutines may continue writing files after the test returns.

Did you encounter this too, or have any thoughts?

@mdisibio Hi! Thanks for reviewing this PR.

t.TempDir() is using os.RemoveAll under the hood (https://github.com/golang/go/blob/go1.17.7/src/testing/testing.go#L966), the main difference is that t.TempDir() calls os.RemoveAll in t.Cleanup. I like the explanation of t.Cleanup vs defer from this StackOverflow answer (https://stackoverflow.com/a/61609670/7902371).

From what I have seen so far in other repositories using t.TempDir(), the "flaky tests" issue you mentioned seem to be happening on Windows machine only. That's because Unix and Windows handle open files differently. Unix permits removing an open file, Windows does not (golang/go#50510 (comment)). A workaround has been implemented (golang/go@d407a8c) to handle the cases where a temporary file is still being opened for a few moments after the test has finished. I don't have a Windows machine so I couldn't test this myself.

My thoughts for t.TempDir are positive. If a test fails because t.TempDir could not complete the cleanup, it might be a good indication that, e.g. our code are not doing graceful shutdown properly, files are not closed after use etc.

[mdisibio]

Ok, ran tests a bunch to check for flakiness. Locally (osx) 10/10 successful, all good there. In GH CI however 1/10 failed directly related to this change: https://github.com/grafana/tempo/runs/5309510668?check_suite_focus=true#step:4:44

--- FAIL: TestSearchWAL (0.52s)
    testing.go:967: TempDir RemoveAll cleanup: unlinkat /tmp/TestSearchWAL600449895/001/blocks: directory not empty
FAIL

[mdisibio]

@Juneezee Please revert the changes to TestSearchWAL (since that failed in the testing, probably flakier than others) and then LGTM. Also thanks for the links and thoughts above, appreciated.

[Juneezee]

@mdisibio Reverted TestSearchWAL. That's weird because I ran the test multiple times on my local Linux machine and still couldn't reproduce the error 🤔 .