Test code review

tests/framework/collector.go

@@ -0,0 +1,77 @@
+package framework
+
+import (
+	"fmt"
+	"os/exec"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+const (
+	CollectorNamespace        = "collector"
+	collectorChartReleaseName = "otel-collector"
+	// FIXME(pleshakov): Find a automated way to keep the version updated here similar to dependabot.
+	// https://github.com/nginxinc/nginx-gateway-fabric/issues/1665
+	collectorChartVersion = "0.73.1"
+)
+
+// InstallCollector installs the otel-collector.
+func InstallCollector() ([]byte, error) {
+	repoAddArgs := []string{
+		"repo",
+		"add",
+		"open-telemetry",
+		"https://open-telemetry.github.io/opentelemetry-helm-charts",
+	}
+
+	if output, err := exec.Command("helm", repoAddArgs...).CombinedOutput(); err != nil {
+		return output, err
+	}
+
+	args := []string{
+		"install",
+		collectorChartReleaseName,
+		"open-telemetry/opentelemetry-collector",
+		"--create-namespace",
+		"--namespace", CollectorNamespace,
+		"--version", collectorChartVersion,
+		"-f", "manifests/telemetry/collector-values.yaml",
+		"--wait",
+	}
+
+	return exec.Command("helm", args...).CombinedOutput()
+}
+
+// UninstallCollector uninstalls the otel-collector.
+func UninstallCollector(resourceManager ResourceManager) ([]byte, error) {
+	args := []string{
+		"uninstall", collectorChartReleaseName,
+		"--namespace", CollectorNamespace,
+	}
+
+	output, err := exec.Command("helm", args...).CombinedOutput()
+	if err != nil {
+		return output, err
+	}
+
+	return nil, resourceManager.DeleteNamespace(CollectorNamespace)
+}
+
+// GetCollectorPodName returns the name of the collector Pod.
+func GetCollectorPodName(resourceManager ResourceManager) (string, error) {
+	collectorPodNames, err := resourceManager.GetPodNames(
+		CollectorNamespace,
+		client.MatchingLabels{
+			"app.kubernetes.io/name": "opentelemetry-collector",
+		},
+	)
+	if err != nil {
+		return "", err
+	}
+
+	if len(collectorPodNames) != 1 {
+		return "", fmt.Errorf("expected 1 collector pod, got %d", len(collectorPodNames))
+	}
+
+	return collectorPodNames[0], nil
+}

tests/framework/request.go

@@ -6,13 +6,10 @@ import (
 	"crypto/tls"
 	"fmt"
 	"io"
-	"log"
 	"net"
 	"net/http"
 	"strings"
 	"time"
-
-	"k8s.io/apimachinery/pkg/util/wait"
 )
 
 // Get sends a GET request to the specified url.
@@ -54,7 +51,7 @@ func makeRequest(method, url, address string, body io.Reader, timeout time.Durat
 		return dialer.DialContext(ctx, network, fmt.Sprintf("%s:%s", address, port))
 	}
 
-	ctx, cancel := context.WithTimeout(context.Background(), requestTimeout)
+	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
 
 	req, err := http.NewRequestWithContext(ctx, method, url, body)
@@ -80,36 +77,3 @@ func makeRequest(method, url, address string, body io.Reader, timeout time.Durat
 
 	return resp, nil
 }
-
-// GetWithRetry retries the Get function until it succeeds or the context times out.
-func GetWithRetry(
-	ctx context.Context,
-	url,
-	address string,
-	requestTimeout time.Duration,
-) (int, string, error) {
-	var statusCode int
-	var body string
-
-	err := wait.PollUntilContextCancel(
-		ctx,
-		500*time.Millisecond,
-		true, /* poll immediately */
-		func(ctx context.Context) (bool, error) {
-			var getErr error
-			statusCode, body, getErr = Get(url, address, requestTimeout)
-			if getErr != nil {
-				return false, getErr
-			}
-
-			if statusCode != 200 {
-				log.Printf("got %d code instead of expected 200\n", statusCode)
-				return false, nil
-			}
-
-			return true, nil
-		},
-	)
-
-	return statusCode, body, err
-}

tests/suite/sample_test.go

@@ -1,10 +1,10 @@
 package suite
 
 import (
-	"context"
 	"fmt"
 	"net/http"
 	"strconv"
+	"strings"
 	"time"
 
 	. "github.com/onsi/ginkgo/v2"
@@ -48,12 +48,23 @@ var _ = Describe("Basic test example", Label("functional"), func() {
 			url = fmt.Sprintf("http://foo.example.com:%s/hello", strconv.Itoa(portFwdPort))
 		}
 
-		ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
-		defer cancel()
-
-		status, body, err := framework.GetWithRetry(ctx, url, address, timeoutConfig.RequestTimeout)
-		Expect(err).ToNot(HaveOccurred())
-		Expect(status).To(Equal(http.StatusOK))
-		Expect(body).To(ContainSubstring("URI: /hello"))
+		Eventually(
+			func() error {
+				status, body, err := framework.Get(url, address, timeoutConfig.RequestTimeout)
+				if err != nil {
+					return err
+				}
+				if status != http.StatusOK {
+					return fmt.Errorf("status not 200; got %d", status)
+				}
+				expBody := "URI: /hello"
+				if !strings.Contains(body, expBody) {
+					return fmt.Errorf("bad body: got %s; expected %s", body, expBody)
+				}
+				return nil
+			}).
+			WithTimeout(timeoutConfig.RequestTimeout).
+			WithPolling(500 * time.Millisecond).
+			Should(Succeed())
 	})
 })

tests/suite/telemetry_test.go

@@ -1,34 +1,22 @@
 package suite
 
 import (
-	"context"
 	"fmt"
-	"os/exec"
 	"strings"
-	"time"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
 	core "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	crClient "sigs.k8s.io/controller-runtime/pkg/client"
-)
 
-const (
-	collectorNamespace        = "collector"
-	collectorChartReleaseName = "otel-collector"
-	// FIXME(pleshakov): Find a automated way to keep the version updated here similar to dependabot.
-	// https://github.com/nginxinc/nginx-gateway-fabric/issues/1665
-	collectorChartVersion = "0.73.1"
+	"github.com/nginxinc/nginx-gateway-fabric/tests/framework"
 )
 
 var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func() {
 	BeforeEach(func() {
 		// Because NGF reports telemetry on start, we need to install the collector first.
 
 		// Install collector
-		output, err := installCollector()
+		output, err := framework.InstallCollector()
 		Expect(err).ToNot(HaveOccurred(), string(output))
 
 		// Install NGF
@@ -41,22 +29,13 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func(
 	})
 
 	AfterEach(func() {
-		output, err := uninstallCollector()
+		output, err := framework.UninstallCollector(resourceManager)
 		Expect(err).ToNot(HaveOccurred(), string(output))
 	})
 
 	It("sends telemetry", func() {
-		names, err := resourceManager.GetPodNames(
-			collectorNamespace,
-			crClient.MatchingLabels{
-				"app.kubernetes.io/name": "opentelemetry-collector",
-			},
-		)
-
+		name, err := framework.GetCollectorPodName(resourceManager)
 		Expect(err).ToNot(HaveOccurred())
-		Expect(names).To(HaveLen(1))
-
-		name := names[0]
 
 		// We assert that all data points were sent
 		// For some data points, as a sanity check, we assert on sent values.
@@ -68,15 +47,15 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func(
 		Expect(err).ToNot(HaveOccurred())
 
 		matchFirstExpectedLine := func() bool {
-			logs, err := resourceManager.GetPodLogs(collectorNamespace, name, &core.PodLogOptions{})
+			logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, name, &core.PodLogOptions{})
 			Expect(err).ToNot(HaveOccurred())
 			return strings.Contains(logs, "dataType: Str(ngf-product-telemetry)")
 		}
 
 		// Wait until the collector has received the telemetry data
 		Eventually(matchFirstExpectedLine, "30s", "5s").Should(BeTrue())
 
-		logs, err := resourceManager.GetPodLogs(collectorNamespace, name, &core.PodLogOptions{})
+		logs, err := resourceManager.GetPodLogs(framework.CollectorNamespace, name, &core.PodLogOptions{})
 		Expect(err).ToNot(HaveOccurred())
 
 		assertConsecutiveLinesInLogs(
@@ -107,54 +86,6 @@ var _ = Describe("Telemetry test with OTel collector", Label("telemetry"), func(
 	})
 })
 
-func installCollector() ([]byte, error) {
-	repoAddArgs := []string{
-		"repo",
-		"add",
-		"open-telemetry",
-		"https://open-telemetry.github.io/opentelemetry-helm-charts",
-	}
-
-	if output, err := exec.Command("helm", repoAddArgs...).CombinedOutput(); err != nil {
-		return output, err
-	}
-
-	args := []string{
-		"install",
-		collectorChartReleaseName,
-		"open-telemetry/opentelemetry-collector",
-		"--create-namespace",
-		"--namespace", collectorNamespace,
-		"--version", collectorChartVersion,
-		"-f", "manifests/telemetry/collector-values.yaml",
-		"--wait",
-	}
-
-	return exec.Command("helm", args...).CombinedOutput()
-}
-
-func uninstallCollector() ([]byte, error) {
-	args := []string{
-		"uninstall", collectorChartReleaseName,
-		"--namespace", collectorNamespace,
-	}
-
-	output, err := exec.Command("helm", args...).CombinedOutput()
-	if err != nil {
-		return output, err
-	}
-
-	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
-	defer cancel()
-
-	err = k8sClient.Delete(ctx, &core.Namespace{ObjectMeta: metav1.ObjectMeta{Name: collectorNamespace}})
-	if err != nil && !apierrors.IsNotFound(err) {
-		return nil, err
-	}
-
-	return nil, resourceManager.DeleteNamespace(collectorNamespace)
-}
-
 func assertConsecutiveLinesInLogs(logs string, expectedLines []string) {
 	lines := strings.Split(logs, "\n")