testutil compareMetricFamilies: make less error-prone (prometheus#1424)

* testutil compareMetricFamilies: make less error-prone

The functions `GatherAndCompare`, `ScrapeAndCompare` and others that use
`compareMetricFamilies` under the hood can return no error if
`metricNames` includes none of the names found in the scraped/gathered
results. To avoid false Positves (an error being the negative case), we
can return an error if there is is at least one name in `metricNames`
that is not in the filtered results.

Fixes: prometheus#1351

Signed-off-by: leonnicolas <[email protected]>

* Add missing metricNames to error

In to see which metric names are missing, we can add them to the error
message.

Signed-off-by: leonnicolas <[email protected]>

* 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 <[email protected]>
Signed-off-by: leonnicolas <[email protected]>

* prometheus/testutil/testutil_test.go: compare complete error

Before we would only compare the error prefix in `TestScrapeAndCompare`.

Signed-off-by: leonnicolas <[email protected]>

---------

Signed-off-by: leonnicolas <[email protected]>
Signed-off-by: leonnicolas <[email protected]>
Co-authored-by: Bartlomiej Plotka <[email protected]>

Author: leonnicolas

prometheus/testutil/testutil.go

@@ -277,6 +277,15 @@ func compareMetricFamilies(got, expected []*dto.MetricFamily, metricNames ...str
 	if metricNames != nil {
 		got = filterMetrics(got, metricNames)
 		expected = filterMetrics(expected, metricNames)
+		if len(metricNames) > len(got) {
+			var missingMetricNames []string
+			for _, name := range metricNames {
+				if ok := hasMetricByName(got, name); !ok {
+					missingMetricNames = append(missingMetricNames, name)
+				}
+			}
+			return fmt.Errorf("expected metric name(s) not found: %v", missingMetricNames)
+		}
 	}
 
 	return compare(got, expected)
@@ -318,3 +327,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
+}

prometheus/testutil/testutil_test.go

@@ -328,27 +328,46 @@ func TestMetricNotFound(t *testing.T) {
 }
 
 func TestScrapeAndCompare(t *testing.T) {
-	const expected = `
+	scenarios := map[string]struct {
+		want        string
+		metricNames []string
+		// expectedErr if empty, means no fail is expected for the comparison.
+		expectedErr string
+	}{
+		"empty metric Names": {
+			want: `
 		# HELP some_total A value that represents a counter.
 		# TYPE some_total counter
 
 		some_total{ label1 = "value1" } 1
-	`
+	`,
+			metricNames: []string{},
+		},
+		"one metric": {
+			want: `
+		# HELP some_total A value that represents a counter.
+		# TYPE some_total counter
 
-	expectedReader := strings.NewReader(expected)
+		some_total{ label1 = "value1" } 1
+	`,
+			metricNames: []string{"some_total"},
+		},
+		"multiple expected": {
+			want: `
+		# HELP some_total A value that represents a counter.
+		# TYPE some_total counter
 
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprintln(w, expected)
-	}))
-	defer ts.Close()
+		some_total{ label1 = "value1" } 1
 
-	if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total"); err != nil {
-		t.Errorf("unexpected scraping result:\n%s", err)
-	}
-}
+		# HELP some_total2 A value that represents a counter.
+		# TYPE some_total2 counter
 
-func TestScrapeAndCompareWithMultipleExpected(t *testing.T) {
-	const expected = `
+		some_total2{ label2 = "value2" } 1
+	`,
+			metricNames: []string{"some_total2"},
+		},
+		"expected metric name is not scraped": {
+			want: `
 		# HELP some_total A value that represents a counter.
 		# TYPE some_total counter
 
@@ -358,53 +377,78 @@ func TestScrapeAndCompareWithMultipleExpected(t *testing.T) {
 		# TYPE some_total2 counter
 
 		some_total2{ label2 = "value2" } 1
-	`
-
-	expectedReader := strings.NewReader(expected)
+	`,
+			metricNames: []string{"some_total3"},
+			expectedErr: "expected metric name(s) not found: [some_total3]",
+		},
+		"one of multiple expected metric names is not scraped": {
+			want: `
+		# HELP some_total A value that represents a counter.
+		# TYPE some_total counter
 
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		fmt.Fprintln(w, expected)
-	}))
-	defer ts.Close()
+		some_total{ label1 = "value1" } 1
 
-	if err := ScrapeAndCompare(ts.URL, expectedReader, "some_total2"); err != nil {
-		t.Errorf("unexpected scraping result:\n%s", err)
-	}
-}
+		# HELP some_total2 A value that represents a counter.
+		# TYPE some_total2 counter
 
-func TestScrapeAndCompareFetchingFail(t *testing.T) {
-	err := ScrapeAndCompare("some_url", strings.NewReader("some expectation"), "some_total")
-	if err == nil {
-		t.Errorf("expected an error but got nil")
+		some_total2{ label2 = "value2" } 1
+	`,
+			metricNames: []string{"some_total1", "some_total3"},
+			expectedErr: "expected metric name(s) not found: [some_total1 some_total3]",
+		},
 	}
-	if !strings.HasPrefix(err.Error(), "scraping metrics failed") {
-		t.Errorf("unexpected error happened: %s", err)
+	for name, scenario := range scenarios {
+		t.Run(name, func(t *testing.T) {
+			expectedReader := strings.NewReader(scenario.want)
+
+			ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				fmt.Fprintln(w, scenario.want)
+			}))
+			defer ts.Close()
+			if err := ScrapeAndCompare(ts.URL, expectedReader, scenario.metricNames...); err != nil {
+				if scenario.expectedErr == "" || err.Error() != scenario.expectedErr {
+					t.Errorf("unexpected error happened: %s", err)
+				}
+			} else if scenario.expectedErr != "" {
+				t.Errorf("expected an error but got nil")
+			}
+		})
 	}
-}
 
-func TestScrapeAndCompareBadStatusCode(t *testing.T) {
-	const expected = `
+	t.Run("fetching fail", func(t *testing.T) {
+		err := ScrapeAndCompare("some_url", strings.NewReader("some expectation"), "some_total")
+		if err == nil {
+			t.Errorf("expected an error but got nil")
+		}
+		if !strings.HasPrefix(err.Error(), "scraping metrics failed") {
+			t.Errorf("unexpected error happened: %s", err)
+		}
+	})
+
+	t.Run("bad status code", func(t *testing.T) {
+		const expected = `
 		# HELP some_total A value that represents a counter.
 		# TYPE some_total counter
 
 		some_total{ label1 = "value1" } 1
 	`
 
-	expectedReader := strings.NewReader(expected)
+		expectedReader := strings.NewReader(expected)
 
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusBadGateway)
-		fmt.Fprintln(w, expected)
-	}))
-	defer ts.Close()
+		ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusBadGateway)
+			fmt.Fprintln(w, expected)
+		}))
+		defer ts.Close()
 
-	err := ScrapeAndCompare(ts.URL, expectedReader, "some_total")
-	if err == nil {
-		t.Errorf("expected an error but got nil")
-	}
-	if !strings.HasPrefix(err.Error(), "the scraping target returned a status code other than 200") {
-		t.Errorf("unexpected error happened: %s", err)
-	}
+		err := ScrapeAndCompare(ts.URL, expectedReader, "some_total")
+		if err == nil {
+			t.Errorf("expected an error but got nil")
+		}
+		if !strings.HasPrefix(err.Error(), "the scraping target returned a status code other than 200") {
+			t.Errorf("unexpected error happened: %s", err)
+		}
+	})
 }
 
 func TestCollectAndCount(t *testing.T) {