testhelper: Fix tests being order-dependent due to shared logging dir

When the `TEST_LOG_DIR` environment variable is set then we write all
logs into the directory pointed to by this variable. This variable is
set in our CI so that we can retrieve more detailed logs in case any of
the tests fails. The way this works though means that if the environment
variable is set, then all tests will write logs to the same log
directory. It is thus essentially shared state between all tests in case
any of the tests happens to verify whether logs have been written.

And of course this happens: `TestFetchInternalRemote_successful` for
example verifies that no log entry is written to "gitaly_hooks.log".
This assertion frequently fails in our CI with an error related to Git's
pre-receive hooks, even though this test doesn't run the pre-receive
hook at all. This is impossible to debug locally though as we don't set
up the shared logging directory by default, so as long as you don't
happen to run with `TEST_LOG_DIR` set up you cannot ever reproduce the
error.

Reduce the scope of this flake by sharding log directories by the test's
name. So instead of writing into the same directory, each test will have
its own log directory now. For one this fixes the flake, but on the
other hand it also makes it so much easier to correlate log entries with
failing tests as they are now clearly scoped by name.

This doesn't fully fix the issue though: if the same test name happens
to exist across two different packages then we would still use the same
log directory. Let's just accept that risk for now though -- it seems
unlikely enough to be an issue in practice. And if it is we should maybe
think about just ripping out the `TEST_LOG_DIR` handling altogether.

1 files changed, 11 insertions, 5 deletions

diff --git a/internal/testhelper/logger.go b/internal/testhelper/logger.go
index ad7cdbcf5..68dfc3b00 100644
--- a/internal/testhelper/logger.go
+++ b/internal/testhelper/logger.go
@@ -46,15 +46,21 @@ func NewGitalyServerLogger(tb testing.TB) *logrus.Logger {
 	return logger
 }
 
-// CreateTestLogDir creates the log directory specified in the `TEST_LOG_DIR` environment variable.
-// If the variable is unset, then no directory will be created. This method returns the path to
-// the log dir if TEST_LOG_DIR is set.
+// CreateTestLogDir creates a new log directory for testing purposes if the environment variable
+// `TEST_LOG_DIR` is set. The log directory will then be created as a subdirectory of the value that
+// `TEST_LOG_DIR` points to. The name of the subdirectory will match the executing test's name.
+//
+// Returns the name of the created log directory. If the environment variable is not set then this
+// functions returns an empty string.
 func CreateTestLogDir(tb testing.TB) string {
 	testLogDir := os.Getenv("TEST_LOG_DIR")
 	if len(testLogDir) == 0 {
 		return ""
 	}
-	require.NoError(tb, os.MkdirAll(testLogDir, 0o755))
 
-	return testLogDir
+	logDir := filepath.Join(testLogDir, tb.Name())
+
+	require.NoError(tb, os.MkdirAll(logDir, 0o755))
+
+	return logDir
 }