chore: refactor tests to fix timeouts and improve cleanup (#422)

A recent refactor of the test code introduced persistent timeouts, even in tests that do not perform API-intensive tasks (for example, read-only tests that request capacity information). These timeouts were happening because the tests temporarily replace stdout with a byte buffer in order to capture command output for validation; during the most recent refactor, the code that closes the byte buffer and restores stdout was moved to a `t.Cleanup` handler, so the buffer was not closed until after the test finished, causing the test to hang until it timed out. This moves the stdout-juggling code into a helper function to ensure that we are using the same logic across all tests. The helper function logs any errors that happen while closing or reading the byte buffer, but does not fail the test for those errors; the tests themselves should fail if those errors impact the behavior under test. Fixes #416. Related to #343. In addition, existing test helpers are refactored to more easily ensure that resources created with test helpers are automatically cleaned up after a test runs and to ensure that tests fail early if a test resource could not be created. Test helpers for deleting resources now log the ID of the resource they are trying to delete so we can more easily triage issues with test cleanup.
equinix · Dec 20, 2023 · 4bfe00f · 4bfe00f
2 parents a25763c + 29881c6
commit 4bfe00f
Show file tree

Hide file tree

Showing 28 changed files with 358 additions and 1,180 deletions.
diff --git a/test/e2e/capacitytest/capacity_test.go b/test/e2e/capacitytest/capacity_test.go
@@ -1,14 +1,13 @@
 package capacitytest
 
 import (
-	"io"
-	"os"
 	"strings"
 	"testing"
 
 	"github.com/equinix/metal-cli/internal/capacity"
 	root "github.com/equinix/metal-cli/internal/cli"
 	outputPkg "github.com/equinix/metal-cli/internal/outputs"
+	"github.com/equinix/metal-cli/test/helper"
 	"github.com/spf13/cobra"
 )
 
@@ -38,19 +37,9 @@ func TestCli_Capacity(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				root.SetArgs([]string{subCommand, "get"})
-				rescueStdout := os.Stdout
-				r, w, _ := os.Pipe()
-				os.Stdout = w
-				t.Cleanup(func() {
-					w.Close()
-					os.Stdout = rescueStdout
-				})
 
-				if err := root.Execute(); err != nil {
-					t.Fatal(err)
-				}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-				out, _ := io.ReadAll(r)
 				if !strings.Contains(string(out[:]), "n3.xlarge.x86") &&
 					!strings.Contains(string(out[:]), "m3.large.x86") &&
 					!strings.Contains(string(out[:]), "s3.xlarge.x86") &&
@@ -71,19 +60,9 @@ func TestCli_Capacity(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				root.SetArgs([]string{subCommand, "get", "-m", "-P", "c3.small.x86"})
-				rescueStdout := os.Stdout
-				r, w, _ := os.Pipe()
-				os.Stdout = w
-				t.Cleanup(func() {
-					w.Close()
-					os.Stdout = rescueStdout
-				})
 
-				if err := root.Execute(); err != nil {
-					t.Fatal(err)
-				}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-				out, _ := io.ReadAll(r)
 				if !strings.Contains(string(out[:]), "c3.small.x86") &&
 					!strings.Contains(string(out[:]), "mt") &&
 					!strings.Contains(string(out[:]), "sv") &&
@@ -104,19 +83,9 @@ func TestCli_Capacity(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				root.SetArgs([]string{subCommand, "get", "-m", "-P", "m3.large.x86"})
-				rescueStdout := os.Stdout
-				r, w, _ := os.Pipe()
-				os.Stdout = w
-				t.Cleanup(func() {
-					w.Close()
-					os.Stdout = rescueStdout
-				})
 
-				if err := root.Execute(); err != nil {
-					t.Fatal(err)
-				}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-				out, _ := io.ReadAll(r)
 				if !strings.Contains(string(out[:]), "m3.large.x86") &&
 					!strings.Contains(string(out[:]), "mt") &&
 					!strings.Contains(string(out[:]), "sv") &&
@@ -136,19 +105,9 @@ func TestCli_Capacity(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				root.SetArgs([]string{subCommand, "check", "-m", "ny,da", "-P", "c3.medium.x86", "-q", "10"})
-				rescueStdout := os.Stdout
-				r, w, _ := os.Pipe()
-				os.Stdout = w
-				t.Cleanup(func() {
-					w.Close()
-					os.Stdout = rescueStdout
-				})
 
-				if err := root.Execute(); err != nil {
-					t.Fatal(err)
-				}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-				out, _ := io.ReadAll(r)
 				if !strings.Contains(string(out[:]), "c3.medium.x86") &&
 					!strings.Contains(string(out[:]), "ny") &&
 					!strings.Contains(string(out[:]), "da") {
@@ -166,19 +125,9 @@ func TestCli_Capacity(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				root.SetArgs([]string{subCommand, "check", "-m", "da", "-P", "c3.medium.x86,m3.large.x86", "-q", "10"})
-				rescueStdout := os.Stdout
-				r, w, _ := os.Pipe()
-				os.Stdout = w
-				t.Cleanup(func() {
-					w.Close()
-					os.Stdout = rescueStdout
-				})
 
-				if err := root.Execute(); err != nil {
-					t.Fatal(err)
-				}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-				out, _ := io.ReadAll(r)
 				if !strings.Contains(string(out[:]), "c3.medium.x86") &&
 					!strings.Contains(string(out[:]), "m3.large.x86") &&
 					!strings.Contains(string(out[:]), "ny") &&
@@ -197,19 +146,9 @@ func TestCli_Capacity(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				root.SetArgs([]string{subCommand, "check", "-m", "ny,da", "-P", "c3.medium.x86,m3.large.x86", "-q", "10"})
-				rescueStdout := os.Stdout
-				r, w, _ := os.Pipe()
-				os.Stdout = w
-				t.Cleanup(func() {
-					w.Close()
-					os.Stdout = rescueStdout
-				})
 
-				if err := root.Execute(); err != nil {
-					t.Fatal(err)
-				}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-				out, _ := io.ReadAll(r)
 				if !strings.Contains(string(out[:]), "c3.medium.x86") &&
 					!strings.Contains(string(out[:]), "m3.large.x86") &&
 					!strings.Contains(string(out[:]), "ny") &&

diff --git a/test/e2e/devices/devicecreateflagstest/device_create_flags_test.go b/test/e2e/devices/devicecreateflagstest/device_create_flags_test.go
@@ -1,8 +1,6 @@
 package devicecreateflagstest
 
 import (
-	"io"
-	"os"
 	"regexp"
 	"strings"
 	"testing"
@@ -15,7 +13,7 @@ import (
 )
 
 func TestCli_Devices_Create_Flags(t *testing.T) {
-	var projectId, deviceId string
+	var deviceId string
 	var err error
 	subCommand := "device"
 	consumerToken := ""
@@ -42,59 +40,35 @@ func TestCli_Devices_Create_Flags(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				projectName := "metal-cli-device-create-flags" + helper.GenerateRandomString(5)
-				projectId, err = helper.CreateTestProject(t, projectName)
-				t.Cleanup(func() {
-					if err := helper.CleanTestProject(t, projectId); err != nil &&
-						!strings.Contains(err.Error(), "Not Found") {
-						t.Error(err)
-					}
-				})
-				if err != nil {
-					t.Fatal(err)
+				project := helper.CreateTestProject(t, projectName)
+
+				root.SetArgs([]string{subCommand, "create", "-p", project.GetId(), "-P", "m3.small.x86", "-m", "da", "-H", "metal-cli-create-flags-dev", "--operating-system", "custom_ipxe", "--always-pxe=true", "--ipxe-script-url", "https://boot.netboot.xyz/"})
+
+				out := helper.ExecuteAndCaptureOutput(t, root)
+
+				if !strings.Contains(string(out[:]), "metal-cli-create-flags-dev") &&
+					!strings.Contains(string(out[:]), "Ubuntu 20.04 LTS") &&
+					!strings.Contains(string(out[:]), "queued") {
+					t.Error("expected output should include metal-cli-create-flags-dev, Ubuntu 20.04 LTS, and queued strings in the out string ")
 				}
+				name := "metal-cli-create-flags-dev"
+				idNamePattern := `(?m)^\| ([a-zA-Z0-9-]+) +\| *` + name + ` *\|`
 
-				if len(projectId) != 0 {
+				// Find the match of the ID and NAME pattern in the table string
+				match := regexp.MustCompile(idNamePattern).FindStringSubmatch(string(out[:]))
 
-					root.SetArgs([]string{subCommand, "create", "-p", projectId, "-P", "m3.small.x86", "-m", "da", "-H", "metal-cli-create-flags-dev", "--operating-system", "custom_ipxe", "--always-pxe=true", "--ipxe-script-url", "https://boot.netboot.xyz/"})
-					rescueStdout := os.Stdout
-					r, w, _ := os.Pipe()
-					os.Stdout = w
+				// Extract the ID from the match
+				if len(match) > 1 {
+					deviceId = strings.TrimSpace(match[1])
+					_, err = helper.IsDeviceStateActive(t, deviceId)
 					t.Cleanup(func() {
-						w.Close()
-						os.Stdout = rescueStdout
+						helper.CleanTestDevice(t, deviceId)
 					})
-
-					if err := root.Execute(); err != nil {
+					if err != nil {
 						t.Fatal(err)
 					}
-
-					out, _ := io.ReadAll(r)
-
-					if !strings.Contains(string(out[:]), "metal-cli-create-flags-dev") &&
-						!strings.Contains(string(out[:]), "Ubuntu 20.04 LTS") &&
-						!strings.Contains(string(out[:]), "queued") {
-						t.Error("expected output should include metal-cli-create-flags-dev, Ubuntu 20.04 LTS, and queued strings in the out string ")
-					}
-					name := "metal-cli-create-flags-dev"
-					idNamePattern := `(?m)^\| ([a-zA-Z0-9-]+) +\| *` + name + ` *\|`
-
-					// Find the match of the ID and NAME pattern in the table string
-					match := regexp.MustCompile(idNamePattern).FindStringSubmatch(string(out[:]))
-
-					// Extract the ID from the match
-					if len(match) > 1 {
-						deviceId = strings.TrimSpace(match[1])
-						_, err = helper.IsDeviceStateActive(t, deviceId)
-						t.Cleanup(func() {
-							if err := helper.CleanTestDevice(t, deviceId); err != nil &&
-								!strings.Contains(err.Error(), "Not Found") {
-								t.Error(err)
-							}
-						})
-						if err != nil {
-							t.Fatal(err)
-						}
-					}
+				} else {
+					t.Errorf("No match found for %v in %v", idNamePattern, string(out[:]))
 				}
 			},
 		},

diff --git a/test/e2e/devices/devicecreatetest/device_create_test.go b/test/e2e/devices/devicecreatetest/device_create_test.go
@@ -1,8 +1,6 @@
 package devicecreatetest
 
 import (
-	"io"
-	"os"
 	"regexp"
 	"strings"
 	"testing"
@@ -15,7 +13,7 @@ import (
 )
 
 func TestCli_Devices_Create(t *testing.T) {
-	var projectId, deviceId string
+	var deviceId string
 	var err error
 	subCommand := "device"
 	consumerToken := ""
@@ -44,60 +42,36 @@ func TestCli_Devices_Create(t *testing.T) {
 			cmdFunc: func(t *testing.T, c *cobra.Command) {
 				root := c.Root()
 				projectName := "metal-cli-device-create" + randomId
-				projectId, err = helper.CreateTestProject(t, projectName)
-				t.Cleanup(func() {
-					if err := helper.CleanTestProject(t, projectId); err != nil &&
-						!strings.Contains(err.Error(), "Not Found") {
-						t.Error(err)
-					}
-				})
-				if err != nil {
-					t.Fatal(err)
-				}
+				project := helper.CreateTestProject(t, projectName)
 
-				if len(projectId) != 0 {
+				deviceName := "metal-cli-create-dev" + randomId
+				root.SetArgs([]string{subCommand, "create", "-p", project.GetId(), "-P", "m3.small.x86", "-m", "da", "-O", "ubuntu_20_04", "-H", deviceName})
 
-					deviceName := "metal-cli-create-dev" + randomId
-					root.SetArgs([]string{subCommand, "create", "-p", projectId, "-P", "m3.small.x86", "-m", "da", "-O", "ubuntu_20_04", "-H", deviceName})
-					rescueStdout := os.Stdout
-					r, w, _ := os.Pipe()
-					os.Stdout = w
-					t.Cleanup(func() {
-						w.Close()
-						os.Stdout = rescueStdout
-					})
-
-					if err := root.Execute(); err != nil {
-						t.Fatal(err)
-					}
+				out := helper.ExecuteAndCaptureOutput(t, root)
 
-					out, _ := io.ReadAll(r)
-
-					if !strings.Contains(string(out[:]), deviceName) &&
-						!strings.Contains(string(out[:]), "Ubuntu 20.04 LTS") &&
-						!strings.Contains(string(out[:]), "queued") {
-						t.Errorf("expected output should include %s, Ubuntu 20.04 LTS, and queued strings in the out string ", deviceName)
-					}
+				if !strings.Contains(string(out[:]), deviceName) &&
+					!strings.Contains(string(out[:]), "Ubuntu 20.04 LTS") &&
+					!strings.Contains(string(out[:]), "queued") {
+					t.Errorf("expected output should include %s, Ubuntu 20.04 LTS, and queued strings in the out string ", deviceName)
+				}
 
-					idNamePattern := `(?m)^\| ([a-zA-Z0-9-]+) +\| *` + deviceName + ` *\|`
+				idNamePattern := `(?m)^\| ([a-zA-Z0-9-]+) +\| *` + deviceName + ` *\|`
 
-					// Find the match of the ID and NAME pattern in the table string
-					match := regexp.MustCompile(idNamePattern).FindStringSubmatch(string(out[:]))
+				// Find the match of the ID and NAME pattern in the table string
+				match := regexp.MustCompile(idNamePattern).FindStringSubmatch(string(out[:]))
 
-					// Extract the ID from the match
-					if len(match) > 1 {
-						deviceId = strings.TrimSpace(match[1])
-						_, err = helper.IsDeviceStateActive(t, deviceId)
-						t.Cleanup(func() {
-							if err := helper.CleanTestDevice(t, deviceId); err != nil &&
-								!strings.Contains(err.Error(), "Not Found") {
-								t.Error(err)
-							}
-						})
-						if err != nil {
-							t.Fatal(err)
-						}
+				// Extract the ID from the match
+				if len(match) > 1 {
+					deviceId = strings.TrimSpace(match[1])
+					_, err = helper.IsDeviceStateActive(t, deviceId)
+					t.Cleanup(func() {
+						helper.CleanTestDevice(t, deviceId)
+					})
+					if err != nil {
+						t.Fatal(err)
 					}
+				} else {
+					t.Errorf("No match found for %v in %v", idNamePattern, string(out[:]))
 				}
 			},
 		},