logging: improve tests

Standardize naming
Use stricter assertions and reduce boilerplate to make the intent of the tests more obvious.

Also explicitly sort the filenames so that the correct files are pruned,
and so that the tests can not flake.

logging/logfile.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sort"
 	"strconv"
 	"strings"
 	"sync"
@@ -107,6 +108,7 @@ func (l *LogFile) pruneFiles() error {
 		return nil
 	}
 
+	sort.Strings(matches)
 	last := len(matches) - l.MaxFiles
 	return removeFiles(matches[:last])
 }

logging/logfile_test.go

@@ -2,135 +2,122 @@ package logging
 
 import (
 	"io/ioutil"
+	"os"
 	"path/filepath"
+	"sort"
 	"testing"
 	"time"
 
-	"github.com/hashicorp/consul/sdk/testutil"
-)
+	"github.com/stretchr/testify/require"
 
-const (
-	testFileName = "Consul.log"
-	testDuration = 50 * time.Millisecond
-	testBytes    = 10
+	"github.com/hashicorp/consul/sdk/testutil"
 )
 
-func TestLogFile_timeRotation(t *testing.T) {
-	tempDir := testutil.TempDir(t, "LogWriterTime")
+func TestLogFile_Rotation_MaxDuration(t *testing.T) {
+	tempDir := testutil.TempDir(t, "")
 	logFile := LogFile{
-		fileName: testFileName,
+		fileName: "consul.log",
 		logPath:  tempDir,
-		duration: testDuration,
+		duration: 50 * time.Millisecond,
 	}
+
 	logFile.Write([]byte("Hello World"))
-	time.Sleep(3 * testDuration)
+	time.Sleep(3 * logFile.duration)
 	logFile.Write([]byte("Second File"))
-	want := 2
-	if got, _ := ioutil.ReadDir(tempDir); len(got) != want {
-		t.Errorf("Expected %d files, got %v file(s)", want, len(got))
-	}
+	require.Len(t, listDir(t, tempDir), 2)
 }
 
 func TestLogFile_openNew(t *testing.T) {
-	tempDir := testutil.TempDir(t, "LogWriterOpen")
-	logFile := LogFile{fileName: testFileName, logPath: tempDir, duration: testDuration}
-	if err := logFile.openNew(); err != nil {
-		t.Errorf("Expected open file %s, got an error (%s)", testFileName, err)
+	logFile := LogFile{
+		fileName: "consul.log",
+		logPath:  testutil.TempDir(t, ""),
+		duration: defaultRotateDuration,
 	}
+	err := logFile.openNew()
+	require.NoError(t, err)
 
-	if _, err := ioutil.ReadFile(logFile.FileInfo.Name()); err != nil {
-		t.Errorf("Expected readable file %s, got an error (%s)", logFile.FileInfo.Name(), err)
-	}
+	msg := "[INFO] Something"
+	_, err = logFile.Write([]byte(msg))
+	require.NoError(t, err)
+
+	content, err := ioutil.ReadFile(logFile.FileInfo.Name())
+	require.NoError(t, err)
+	require.Contains(t, string(content), msg)
 }
 
-func TestLogFile_byteRotation(t *testing.T) {
+func TestLogFile_Rotation_MaxBytes(t *testing.T) {
 	tempDir := testutil.TempDir(t, "LogWriterBytes")
 	logFile := LogFile{
-		fileName: testFileName,
+		fileName: "somefile.log",
 		logPath:  tempDir,
-		MaxBytes: testBytes,
-		duration: 24 * time.Hour,
+		MaxBytes: 10,
+		duration: defaultRotateDuration,
 	}
 	logFile.Write([]byte("Hello World"))
 	logFile.Write([]byte("Second File"))
-	want := 2
-	tempFiles, _ := ioutil.ReadDir(tempDir)
-	if got := tempFiles; len(got) != want {
-		t.Errorf("Expected %d files, got %v file(s)", want, len(got))
-	}
+	require.Len(t, listDir(t, tempDir), 2)
 }
 
-func TestLogFile_deleteArchives(t *testing.T) {
-	tempDir := testutil.TempDir(t, "LogWriteDeleteArchives")
+func TestLogFile_PruneFiles(t *testing.T) {
+	tempDir := testutil.TempDir(t, t.Name())
 	logFile := LogFile{
-		fileName: testFileName,
+		fileName: "consul.log",
 		logPath:  tempDir,
-		MaxBytes: testBytes,
-		duration: 24 * time.Hour,
+		MaxBytes: 10,
+		duration: defaultRotateDuration,
 		MaxFiles: 1,
 	}
 	logFile.Write([]byte("[INFO] Hello World"))
 	logFile.Write([]byte("[INFO] Second File"))
 	logFile.Write([]byte("[INFO] Third File"))
-	want := 2
-	tempFiles, _ := ioutil.ReadDir(tempDir)
-	if got := tempFiles; len(got) != want {
-		t.Errorf("Expected %d files, got %v file(s)", want, len(got))
-		return
-	}
-	for _, tempFile := range tempFiles {
-		var bytes []byte
-		var err error
-		path := filepath.Join(tempDir, tempFile.Name())
-		if bytes, err = ioutil.ReadFile(path); err != nil {
-			t.Errorf(err.Error())
-			return
-		}
-		contents := string(bytes)
-
-		if contents == "[INFO] Hello World" {
-			t.Errorf("Should have deleted the eldest log file")
-			return
-		}
-	}
+
+	logFiles := listDir(t, tempDir)
+	sort.Strings(logFiles)
+	require.Len(t, logFiles, 2)
+
+	content, err := ioutil.ReadFile(filepath.Join(tempDir, logFiles[0]))
+	require.NoError(t, err)
+	require.Contains(t, string(content), "Second File")
+
+	content, err = ioutil.ReadFile(filepath.Join(tempDir, logFiles[1]))
+	require.NoError(t, err)
+	require.Contains(t, string(content), "Third File")
 }
 
-func TestLogFile_deleteArchivesDisabled(t *testing.T) {
+func TestLogFile_PruneFiles_Disabled(t *testing.T) {
 	tempDir := testutil.TempDir(t, t.Name())
 	logFile := LogFile{
-		fileName: testFileName,
+		fileName: "somename.log",
 		logPath:  tempDir,
-		MaxBytes: testBytes,
-		duration: 24 * time.Hour,
+		MaxBytes: 10,
+		duration: defaultRotateDuration,
 		MaxFiles: 0,
 	}
 	logFile.Write([]byte("[INFO] Hello World"))
 	logFile.Write([]byte("[INFO] Second File"))
 	logFile.Write([]byte("[INFO] Third File"))
-	want := 3
-	tempFiles, _ := ioutil.ReadDir(tempDir)
-	if got := tempFiles; len(got) != want {
-		t.Errorf("Expected %d files, got %v file(s)", want, len(got))
-		return
-	}
+	require.Len(t, listDir(t, tempDir), 3)
 }
 
-func TestLogFile_rotationDisabled(t *testing.T) {
+func TestLogFile_FileRotation_Disabled(t *testing.T) {
 	tempDir := testutil.TempDir(t, t.Name())
 	logFile := LogFile{
-		fileName: testFileName,
+		fileName: "consul.log",
 		logPath:  tempDir,
-		MaxBytes: testBytes,
-		duration: 24 * time.Hour,
+		MaxBytes: 10,
 		MaxFiles: -1,
 	}
 	logFile.Write([]byte("[INFO] Hello World"))
 	logFile.Write([]byte("[INFO] Second File"))
 	logFile.Write([]byte("[INFO] Third File"))
-	want := 1
-	tempFiles, _ := ioutil.ReadDir(tempDir)
-	if got := tempFiles; len(got) != want {
-		t.Errorf("Expected %d files, got %v file(s)", want, len(got))
-		return
-	}
+	require.Len(t, listDir(t, tempDir), 1)
+}
+
+func listDir(t *testing.T, name string) []string {
+	t.Helper()
+	fh, err := os.Open(name)
+	require.NoError(t, err)
+	files, err := fh.Readdirnames(100)
+	require.NoError(t, err)
+	return files
 }