integrationtests: address review comments on cleanup tests

- Remove tautological temp dir tests that self-cleaned in script
- Add TestCleanupLeakedWorkloadTempDirCaughtByAssertion that creates
  a dir without cleanup and verifies detection catches it
- Inline snapshotWorkloadTempDirs into listWorkloadTempDirs
- Add comment explaining assertNoNewWorkloadTempDirs cleanup behavior
- Add non-empty check in OutputDir test to prevent vacuous pass
- Fix .collapsed -> .collapsed.zst suffix consistency

Amp-Thread-ID: https://ampcode.com/threads/T-019c8172-052c-74fe-8d8d-34a0529d082c
Co-authored-by: Amp <amp@ampcode.com>

1 files changed, 47 insertions, 65 deletions

diff --git a/integrationtests/cleanup_test.go b/integrationtests/cleanup_test.go
index cabd735..6fecd10 100644
--- a/integrationtests/cleanup_test.go
+++ b/integrationtests/cleanup_test.go
@@ -10,12 +10,6 @@ import (
 // workloadTempDirPrefix is the prefix used by ioworkload's makeTempDir.
 const workloadTempDirPrefix = "ioworkload-"
 
-// snapshotWorkloadTempDirs returns a set of existing ioworkload temp dirs.
-func snapshotWorkloadTempDirs(t *testing.T) map[string]struct{} {
-	t.Helper()
-	return listWorkloadTempDirs(t)
-}
-
 // listWorkloadTempDirs returns the set of ioworkload-* directories in the
 // system temp directory.
 func listWorkloadTempDirs(t *testing.T) map[string]struct{} {
@@ -36,7 +30,8 @@ func listWorkloadTempDirs(t *testing.T) map[string]struct{} {
 }
 
 // assertNoNewWorkloadTempDirs fails if any new ioworkload temp dirs appeared
-// since the snapshot was taken.
+// since the snapshot was taken. It also cleans up any leaked dirs to prevent
+// cascading failures in subsequent tests.
 func assertNoNewWorkloadTempDirs(t *testing.T, before map[string]struct{}) {
 	t.Helper()
 	after := listWorkloadTempDirs(t)
@@ -58,9 +53,7 @@ func TestCleanupOutputDirContainsOnlyExpectedFiles(t *testing.T) {
 	tmpDir := t.TempDir()
 	outputDir := t.TempDir()
 
-	// Fake workload that prints PID and exits cleanly.
 	workloadBin := writeScript(t, tmpDir, "workload", `echo $$`)
-	// Fake ior that creates an .ior.zst file and a .collapsed file.
 	iorBin := writeScript(t, tmpDir, "ior",
 		`touch test.ior.zst test.collapsed.zst test.svg`)
 
@@ -80,7 +73,10 @@ func TestCleanupOutputDirContainsOnlyExpectedFiles(t *testing.T) {
 		t.Fatalf("read output dir: %v", err)
 	}
 
-	// All files should be inside outputDir (which is t.TempDir(), auto-cleaned).
+	if len(entries) == 0 {
+		t.Fatal("expected files in output dir, got none")
+	}
+
 	for _, e := range entries {
 		name := e.Name()
 		validSuffix := strings.HasSuffix(name, ".ior.zst") ||
@@ -93,39 +89,38 @@ func TestCleanupOutputDirContainsOnlyExpectedFiles(t *testing.T) {
 	}
 }
 
-func TestCleanupNoWorkloadTempDirsLeakedOnSuccess(t *testing.T) {
-	tmpDir := t.TempDir()
-	outputDir := t.TempDir()
-
-	before := snapshotWorkloadTempDirs(t)
-
-	// Fake workload that creates a temp dir, cleans it up, then exits.
-	// This mimics the real ioworkload's makeTempDir + defer cleanup pattern.
-	workloadBin := writeScript(t, tmpDir, "workload",
-		`echo $$; d=$(mktemp -d /tmp/ioworkload-cleanup-test-XXXXXX); rm -rf "$d"`)
-	iorBin := writeScript(t, tmpDir, "ior", `exit 0`)
+func TestCleanupDetectsLeakedWorkloadTempDir(t *testing.T) {
+	before := listWorkloadTempDirs(t)
 
-	h := TestHarness{
-		IorBinary:      iorBin,
-		WorkloadBinary: workloadBin,
-		BpfObject:      filepath.Join(tmpDir, "fake.bpf.o"),
-		OutputDir:      outputDir,
+	// Create a temp dir that looks like a leaked ioworkload dir.
+	leaked, err := os.MkdirTemp("", workloadTempDirPrefix+"leak-test-")
+	if err != nil {
+		t.Fatalf("create leaked dir: %v", err)
 	}
+	defer os.RemoveAll(leaked)
 
-	h.Run("test", 5) //nolint:errcheck
-
-	assertNoNewWorkloadTempDirs(t, before)
+	after := listWorkloadTempDirs(t)
+	var found bool
+	for dir := range after {
+		if _, existed := before[dir]; !existed {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Error("leak detection failed: new ioworkload temp dir was not detected")
+	}
 }
 
-func TestCleanupNoWorkloadTempDirsLeakedOnFailure(t *testing.T) {
+func TestCleanupLeakedWorkloadTempDirCaughtByAssertion(t *testing.T) {
+	before := listWorkloadTempDirs(t)
 	tmpDir := t.TempDir()
 	outputDir := t.TempDir()
 
-	before := snapshotWorkloadTempDirs(t)
-
-	// Fake workload that creates a temp dir, cleans it up, then exits with error.
+	// Fake workload that creates a temp dir but does NOT clean it up.
+	// This simulates a killed workload whose defer cleanup() never ran.
 	workloadBin := writeScript(t, tmpDir, "workload",
-		`echo $$; d=$(mktemp -d /tmp/ioworkload-cleanup-test-XXXXXX); rm -rf "$d"; exit 1`)
+		`echo $$; mktemp -d /tmp/ioworkload-leaked-test-XXXXXX >/dev/null`)
 	iorBin := writeScript(t, tmpDir, "ior", `exit 0`)
 
 	h := TestHarness{
@@ -137,7 +132,21 @@ func TestCleanupNoWorkloadTempDirsLeakedOnFailure(t *testing.T) {
 
 	h.Run("test", 5) //nolint:errcheck
 
-	assertNoNewWorkloadTempDirs(t, before)
+	// Verify that a leaked dir IS detected.
+	after := listWorkloadTempDirs(t)
+	var leaked []string
+	for dir := range after {
+		if _, existed := before[dir]; !existed {
+			leaked = append(leaked, dir)
+		}
+	}
+	if len(leaked) == 0 {
+		t.Error("expected to detect leaked ioworkload temp dir, found none")
+	}
+	// Clean up the intentionally leaked dir.
+	for _, dir := range leaked {
+		os.RemoveAll(dir)
+	}
 }
 
 func TestCleanupOutputDirEmptyAfterIorFailure(t *testing.T) {
@@ -145,7 +154,6 @@ func TestCleanupOutputDirEmptyAfterIorFailure(t *testing.T) {
 	outputDir := t.TempDir()
 
 	workloadBin := writeScript(t, tmpDir, "workload", `echo $$`)
-	// Fake ior that fails without producing any output files.
 	iorBin := writeScript(t, tmpDir, "ior", `exit 1`)
 
 	h := TestHarness{
@@ -173,38 +181,12 @@ func TestCleanupOutputDirEmptyAfterIorFailure(t *testing.T) {
 	}
 }
 
-func TestCleanupDetectsLeakedWorkloadTempDir(t *testing.T) {
-	before := snapshotWorkloadTempDirs(t)
-
-	// Create a temp dir that looks like a leaked ioworkload dir.
-	leaked, err := os.MkdirTemp("", workloadTempDirPrefix+"leak-test-")
-	if err != nil {
-		t.Fatalf("create leaked dir: %v", err)
-	}
-	defer os.RemoveAll(leaked)
-
-	after := listWorkloadTempDirs(t)
-	var found bool
-	for dir := range after {
-		if _, existed := before[dir]; !existed {
-			found = true
-			break
-		}
-	}
-	if !found {
-		t.Error("assertNoNewWorkloadTempDirs should detect the leaked dir")
-	}
-}
-
 func TestCleanupNoArtifactsOutsideOutputDir(t *testing.T) {
 	tmpDir := t.TempDir()
 	outputDir := t.TempDir()
 
-	// Fake workload that prints PID and exits.
 	workloadBin := writeScript(t, tmpDir, "workload", `echo $$`)
-	// Fake ior that creates output files only in its working directory (outputDir).
-	iorBin := writeScript(t, tmpDir, "ior",
-		`touch test.ior.zst`)
+	iorBin := writeScript(t, tmpDir, "ior", `touch test.ior.zst`)
 
 	h := TestHarness{
 		IorBinary:      iorBin,
@@ -215,14 +197,14 @@ func TestCleanupNoArtifactsOutsideOutputDir(t *testing.T) {
 
 	h.Run("test", 5) //nolint:errcheck
 
-	// Verify no .ior.zst files were created in the test's tmpDir (script dir).
+	// Verify no artifacts were created in the script dir (outside outputDir).
 	entries, err := os.ReadDir(tmpDir)
 	if err != nil {
 		t.Fatalf("read tmp dir: %v", err)
 	}
 	for _, e := range entries {
 		if strings.HasSuffix(e.Name(), ".ior.zst") ||
-			strings.HasSuffix(e.Name(), ".collapsed") ||
+			strings.HasSuffix(e.Name(), ".collapsed.zst") ||
 			strings.HasSuffix(e.Name(), ".svg") {
 			t.Errorf("artifact leaked to script dir: %s", e.Name())
 		}