fix(tests): captureOutput refactored to prevent potential deadlocks

Signed-off-by: sbailey <1661003+spbsoluble@users.noreply.github.com>

Author: spbsoluble

cmd/stores_test.go

@@ -17,11 +17,12 @@ package cmd
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/stretchr/testify/assert"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func Test_Stores_HelpCmd(t *testing.T) {
@@ -51,11 +52,13 @@ func Test_Stores_HelpCmd(t *testing.T) {
 func Test_Stores_ListCmd(t *testing.T) {
 	testCmd := RootCmd
 	// test
-	testCmd.SetArgs([]string{"stores", "list", "--exp"})
-	output := captureOutput(func() {
-		err := testCmd.Execute()
-		assert.NoError(t, err)
-	})
+	testCmd.SetArgs([]string{"stores", "list"})
+	output := captureOutput(
+		func() {
+			err := testCmd.Execute()
+			assert.NoError(t, err)
+		},
+	)
 	var stores []interface{}
 	if err := json.Unmarshal([]byte(output), &stores); err != nil {
 		t.Fatalf("Error unmarshalling JSON: %v", err)
@@ -80,10 +83,12 @@ func Test_Stores_GetCmd(t *testing.T) {
 	testCmd := RootCmd
 	// test
 	testCmd.SetArgs([]string{"stores", "list", "--exp"})
-	output := captureOutput(func() {
-		err := testCmd.Execute()
-		assert.NoError(t, err)
-	})
+	output := captureOutput(
+		func() {
+			err := testCmd.Execute()
+			assert.NoError(t, err)
+		},
+	)
 	var stores []interface{}
 	if err := json.Unmarshal([]byte(output), &stores); err != nil {
 		t.Fatalf("Error unmarshalling JSON: %v", err)
@@ -102,10 +107,12 @@ func Test_Stores_GetCmd(t *testing.T) {
 			assert.NotEmpty(t, store.(map[string]interface{})["CertStoreType"])
 
 			testCmd.SetArgs([]string{"stores", "get", "--id", store.(map[string]interface{})["Id"].(string)})
-			output = captureOutput(func() {
-				err := testCmd.Execute()
-				assert.NoError(t, err)
-			})
+			output = captureOutput(
+				func() {
+					err := testCmd.Execute()
+					assert.NoError(t, err)
+				},
+			)
 
 			var store map[string]interface{}
 			if err := json.Unmarshal([]byte(output), &store); err != nil {
@@ -183,11 +190,24 @@ func Test_Stores_ImportCmd(t *testing.T) {
 
 		testCmd := RootCmd
 		// test
-		testCmd.SetArgs([]string{"stores", "import", "csv", "--file", outFileName, "--store-type-name", "k8ssecret", "--exp"})
-		output := captureOutput(func() {
-			err := testCmd.Execute()
-			assert.NoError(t, err)
-		})
+		testCmd.SetArgs(
+			[]string{
+				"stores",
+				"import",
+				"csv",
+				"--file",
+				outFileName,
+				"--store-type-name",
+				"k8ssecret",
+				"--exp",
+			},
+		)
+		output := captureOutput(
+			func() {
+				err := testCmd.Execute()
+				assert.NoError(t, err)
+			},
+		)
 
 		assert.Contains(t, output, "records processed")
 		assert.Contains(t, output, "results written to")
@@ -234,14 +254,16 @@ func Test_Stores_GenerateImportTemplateCmd(t *testing.T) {
 	testCmd := RootCmd
 	// test
 	testCmd.SetArgs([]string{"stores", "import", "generate-template", "--store-type-name", "k8ssecret"})
-	output := captureOutput(func() {
-		err := testCmd.Execute()
-		assert.NoError(t, err)
-		if err != nil {
-			t.Errorf("Error: %v", err)
-			return
-		}
-	})
+	output := captureOutput(
+		func() {
+			err := testCmd.Execute()
+			assert.NoError(t, err)
+			if err != nil {
+				t.Errorf("Error: %v", err)
+				return
+			}
+		},
+	)
 
 	assert.Contains(t, output, "Template file for store type with id")
 	assert.Contains(t, output, "csv")
@@ -312,105 +334,117 @@ func testExportStore(t *testing.T, storeTypeName string) (string, []string) {
 		files  []string
 		err    error
 	)
-	t.Run(fmt.Sprintf("Export Stores of type %s", storeTypeName), func(t *testing.T) {
-		testCmd := RootCmd
-		testCmd.SetArgs([]string{"stores", "export", "--store-type-name", storeTypeName})
-		output = captureOutput(func() {
-			err := testCmd.Execute()
-			assert.NoError(t, err)
-		})
-
-		// assert that output is not empty
-		assert.NotEmpty(t, output)
-
-		// assert that output is a string
-		assert.IsType(t, "", output)
-
-		// assert that output does not contain 'error'
-		assert.NotContains(t, output, "error")
-
-		// assert that output does not contain 'Error'
-		assert.NotContains(t, output, "Error")
-
-		// assert that output does not contain 'ERROR'
-		assert.NotContains(t, output, "ERROR")
-
-		// assert that contains "exported for store type with id"
-		assert.Contains(t, output, "exported for store type with id")
-
-		// assert that contains .csv
-		assert.Contains(t, output, ".csv")
-
-		// assert that a csv file was created in current working directory with a filename that contains 'export_store_*.csv'
-		files, err = findMatchingFiles("*stores_export*.csv")
-		assert.Nil(t, err)
-		assert.NotEmpty(t, files)
-	})
+	t.Run(
+		fmt.Sprintf("Export Stores of type %s", storeTypeName), func(t *testing.T) {
+			testCmd := RootCmd
+			testCmd.SetArgs([]string{"stores", "export", "--store-type-name", storeTypeName})
+			output = captureOutput(
+				func() {
+					err := testCmd.Execute()
+					assert.NoError(t, err)
+				},
+			)
+
+			// assert that output is not empty
+			assert.NotEmpty(t, output)
+
+			// assert that output is a string
+			assert.IsType(t, "", output)
+
+			// assert that output does not contain 'error'
+			assert.NotContains(t, output, "error")
+
+			// assert that output does not contain 'Error'
+			assert.NotContains(t, output, "Error")
+
+			// assert that output does not contain 'ERROR'
+			assert.NotContains(t, output, "ERROR")
+
+			// assert that contains "exported for store type with id"
+			assert.Contains(t, output, "exported for store type with id")
+
+			// assert that contains .csv
+			assert.Contains(t, output, ".csv")
+
+			// assert that a csv file was created in current working directory with a filename that contains 'export_store_*.csv'
+			files, err = findMatchingFiles("*stores_export*.csv")
+			assert.Nil(t, err)
+			assert.NotEmpty(t, files)
+		},
+	)
 	return output, files
 }
 
 func deleteStoreTest(t *testing.T, storeID string, allowFail bool) {
-	t.Run(fmt.Sprintf("Delete Store %s", storeID), func(t *testing.T) {
-		testCmd := RootCmd
-		testCmd.SetArgs([]string{"stores", "delete", "--id", storeID})
-		deleteStoreOutput := captureOutput(func() {
-			err := testCmd.Execute()
+	t.Run(
+		fmt.Sprintf("Delete Store %s", storeID), func(t *testing.T) {
+			testCmd := RootCmd
+			testCmd.SetArgs([]string{"stores", "delete", "--id", storeID})
+			deleteStoreOutput := captureOutput(
+				func() {
+					err := testCmd.Execute()
+					if !allowFail {
+						assert.NoError(t, err)
+					}
+				},
+			)
 			if !allowFail {
-				assert.NoError(t, err)
-			}
-		})
-		if !allowFail {
-			if strings.Contains(deleteStoreOutput, "does not exist") {
-				t.Errorf("Store %s does not exist", storeID)
-			}
-			if strings.Contains(deleteStoreOutput, "cannot be deleted") {
-				assert.Fail(t, fmt.Sprintf("Store %s already exists", storeID))
+				if strings.Contains(deleteStoreOutput, "does not exist") {
+					t.Errorf("Store %s does not exist", storeID)
+				}
+				if strings.Contains(deleteStoreOutput, "cannot be deleted") {
+					assert.Fail(t, fmt.Sprintf("Store %s already exists", storeID))
+				}
+				if strings.Contains(deleteStoreOutput, "error processing the request") {
+					assert.Fail(t, fmt.Sprintf("Store %s was not deleted: %s", storeID, deleteStoreOutput))
+				}
+				assert.Contains(t, deleteStoreOutput, "deleted")
+				assert.Contains(t, deleteStoreOutput, storeID)
 			}
-			if strings.Contains(deleteStoreOutput, "error processing the request") {
-				assert.Fail(t, fmt.Sprintf("Store %s was not deleted: %s", storeID, deleteStoreOutput))
-			}
-			assert.Contains(t, deleteStoreOutput, "deleted")
-			assert.Contains(t, deleteStoreOutput, storeID)
-		}
-	})
+		},
+	)
 }
 
 func testValidateCSVHeader(t *testing.T, filename string, header []string, expected []string) {
 	// iterate bulkStoreImportCSVHeader and verify that each header is in the csv header
-	t.Run(fmt.Sprintf("Validate CSV header %s", filename), func(t *testing.T) {
-		// Check that first col isn't empty
-		assert.NotEmpty(t, header[0], "First column of CSV is empty")
-
-		for _, h := range expected {
-			if h != "Properties" {
-				assert.Contains(t, header, h)
+	t.Run(
+		fmt.Sprintf("Validate CSV header %s", filename), func(t *testing.T) {
+			// Check that first col isn't empty
+			assert.NotEmpty(t, header[0], "First column of CSV is empty")
+
+			for _, h := range expected {
+				if h != "Properties" {
+					assert.Contains(t, header, h)
+				}
 			}
-		}
 
-		var props []string
-		for _, h := range header {
-			if strings.Contains(h, "Properties") {
-				props = append(props, h)
+			var props []string
+			for _, h := range header {
+				if strings.Contains(h, "Properties") {
+					props = append(props, h)
+				}
 			}
-		}
-		assert.NotEmpty(t, props)
-	})
+			assert.NotEmpty(t, props)
+		},
+	)
 }
 
 func testDeleteImportResults(t *testing.T, fileName string) {
 	// get filename from path
 	fName := filepath.Base(fileName)
-	t.Run(fmt.Sprintf("Deleting imported stores %s", fName), func(t *testing.T) {
-		csvData, err := csvToMap(fileName)
-		assert.NoError(t, err)
+	t.Run(
+		fmt.Sprintf("Deleting imported stores %s", fName), func(t *testing.T) {
+			csvData, err := csvToMap(fileName)
+			assert.NoError(t, err)
 
-		for _, row := range csvData {
-			// assert that each row has an ID
-			//assert.NotEmpty(t, row[""])
-			// delete store
-			if row["Id"] != "" && row["Id"] != "error" && row["Id"] != "Id" {
-				deleteStoreTest(t, row["Id"], false)
+			for _, row := range csvData {
+				// assert that each row has an ID
+				//assert.NotEmpty(t, row[""])
+				// delete store
+				if row["Id"] != "" && row["Id"] != "error" && row["Id"] != "Id" {
+					deleteStoreTest(t, row["Id"], false)
+				}
 			}
-		}
-	})
+		},
+	)
 }

cmd/test.go

@@ -21,17 +21,39 @@ import (
 )
 
 func captureOutput(f func()) string {
+	// Save the original os.Stdout
 	old := os.Stdout
+	// Create a pipe
 	r, w, _ := os.Pipe()
+	// Set os.Stdout to the write end of the pipe
 	os.Stdout = w
 
+	// Create a channel to signal when f() has completed
+	done := make(chan bool)
+
+	// Buffer to store the output
+	var buf bytes.Buffer
+
+	// Start a goroutine to copy from the read end of the pipe to the buffer
+	go func() {
+		io.Copy(&buf, r)
+		// Signal that the copying is done
+		done <- true
+	}()
+
+	// Run the provided function f
 	f()
 
+	// Close the write end of the pipe to signal EOF to the reader
 	w.Close()
+
+	// Wait for the goroutine to finish copying
+	<-done
+
+	// Restore the original os.Stdout
 	os.Stdout = old
 
-	var buf bytes.Buffer
-	io.Copy(&buf, r)
+	// Return the captured output as a string
 	return buf.String()
 }