From 437f98868066b61056162700c4c5c61d4b32fce6 Mon Sep 17 00:00:00 2001
From: leonnicolas <60091705+leonnicolas@users.noreply.github.com>
Date: Wed, 7 Feb 2024 18:46:34 +0100
Subject: [PATCH] Apply suggestions from code review

- remove if nil check
- use two nested loops instead of map
- use new function `hasMetricByName` for readability

Co-authored-by: Bartlomiej Plotka <bwplotka@gmail.com>
Signed-off-by: leonnicolas <60091705+leonnicolas@users.noreply.github.com>
---
 prometheus/testutil/testutil.go      | 18 ++++++++++--------
 prometheus/testutil/testutil_test.go | 18 ++++++++----------
 2 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/prometheus/testutil/testutil.go b/prometheus/testutil/testutil.go
index b1cfdbd9d..9668a8a2a 100644
--- a/prometheus/testutil/testutil.go
+++ b/prometheus/testutil/testutil.go
@@ -255,16 +255,9 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str
 		got = filterMetrics(got, metricNames)
 		expected = filterMetrics(expected, metricNames)
 		if len(metricNames) > len(got) {
-			h := make(map[string]struct{})
-			for _, mf := range got {
-				if mf == nil {
-					continue
-				}
-				h[mf.GetName()] = struct{}{}
-			}
 			var missingMetricNames []string
 			for _, name := range metricNames {
-				if _, ok := h[name]; !ok {
+				if ok := hasMetricByName(got, name); !ok {
 					missingMetricNames = append(missingMetricNames, name)
 				}
 			}
@@ -372,3 +365,12 @@ func filterMetrics(metrics []*dto.MetricFamily, names []string) []*dto.MetricFam
 	}
 	return filtered
 }
+
+func hasMetricByName(metrics []*dto.MetricFamily, name string) bool {
+	for _, mf := range metrics {
+		if mf.GetName() == name {
+			return true
+		}
+	}
+	return false
+}
diff --git a/prometheus/testutil/testutil_test.go b/prometheus/testutil/testutil_test.go
index bfe0f2c96..62712e4a3 100644
--- a/prometheus/testutil/testutil_test.go
+++ b/prometheus/testutil/testutil_test.go
@@ -335,8 +335,8 @@ func TestScrapeAndCompare(t *testing.T) {
 	scenarios := map[string]struct {
 		want        string
 		metricNames []string
-		errPrefix   string
-		fail        bool
+		// expectedErrPrefix if empty, means no fail is expected for the comparison.
+		expectedErrPrefix string
 	}{
 		"empty metric Names": {
 			want: `
@@ -382,9 +382,8 @@ func TestScrapeAndCompare(t *testing.T) {
 
 		some_total2{ label2 = "value2" } 1
 	`,
-			metricNames: []string{"some_total3"},
-			errPrefix:   "expected metric name(s) not found",
-			fail:        true,
+			metricNames:       []string{"some_total3"},
+			expectedErrPrefix: "expected metric name(s) not found",
 		},
 		"one of multiple expected metric names is not scraped": {
 			want: `
@@ -398,9 +397,8 @@ func TestScrapeAndCompare(t *testing.T) {
 
 		some_total2{ label2 = "value2" } 1
 	`,
-			metricNames: []string{"some_total1", "some_total3"},
-			errPrefix:   "expected metric name(s) not found",
-			fail:        true,
+			metricNames:       []string{"some_total1", "some_total3"},
+			expectedErrPrefix: "expected metric name(s) not found",
 		},
 	}
 	for name, scenario := range scenarios {
@@ -412,10 +410,10 @@ func TestScrapeAndCompare(t *testing.T) {
 			}))
 			defer ts.Close()
 			if err := ScrapeAndCompare(ts.URL, expectedReader, scenario.metricNames...); err != nil {
-				if !scenario.fail || !strings.HasPrefix(err.Error(), scenario.errPrefix) {
+				if scenario.expectedErrPrefix == "" || !strings.HasPrefix(err.Error(), scenario.expectedErrPrefix) {
 					t.Errorf("unexpected error happened: %s", err)
 				}
-			} else if scenario.fail {
+			} else if scenario.expectedErrPrefix != "" {
 				t.Errorf("expected an error but got nil")
 			}
 		})