Replace t.Fatal with require in Prometheus package #6690

AshwinSriram11 · 2024-06-18T11:39:41Z

Description

Continuation to #6659, #6680 and #6683.

Type of change

What type of changes does your code introduce to the Prometheus operator? Put an x in the box that apply.

CHANGE (fix or feature that would cause existing functionality to not work as expected)
FEATURE (non-breaking change which adds functionality)
BUGFIX (non-breaking change which fixes an issue)
ENHANCEMENT (non-breaking change which improves existing functionality)
NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

AshwinSriram11 · 2024-06-18T12:02:49Z

pkg/prometheus/server/statefulset_test.go

-		if !found {
-			t.Fatalf("--tsdb.path argument should be given to the Thanos sidecar, got %q", strings.Join(sset.Spec.Template.Spec.Containers[3].Args, " "))
-		}
+		require.True(t, found, "--tsdb.path argument should be given to the Thanos sidecar, got %q", strings.Join(sset.Spec.Template.Spec.Containers[3].Args, " "))


I checked why the tests are failing and I got this error

--- FAIL: TestThanosObjectStorageFile (0.00s) panic: runtime error: index out of range [3] with length 3 [recovered] panic: runtime error: index out of range [3] with length 3

When I changed it to sset.Spec.Template.Spec.Containers[2].Args, it was working fine. Wdyt about this?

AshwinSriram11 · 2024-06-18T12:08:28Z

pkg/prometheus/operator_test.go

+			if !test.expectErr {
+				require.NoError(t, err)


This might be better. Wdyt?

require.False(t,(err!=nil && !test.expectErr),"unexpected error occurred: %v", err)

I think it looks good the way it is now, we just don't need the second if because, if we get there, it's always true.

What do you think about the suggestion below?

for _, c := range cases { test := c t.Run(test.name, func(t *testing.T) { err := ValidateRemoteWriteSpec(test.spec) if test.expectErr { require.Error(t, err) return } require.NoError(t, err) }) }

AshwinSriram11 · 2024-06-18T12:29:21Z

pkg/prometheus/agent/statefulset_test.go

-			if test.shouldContain {
-				t.Fatalf("expected Prometheus args to contain %v, but got %v", test.expectedArg, promArgs)
-			} else {
-				t.Fatalf("expected Prometheus args to NOT contain %v, but got %v", test.expectedArg, promArgs)
-			}
+			require.False(t, test.shouldContain, "expected Prometheus args to contain %v, but got %v", test.expectedArg, promArgs)
+			require.True(t, test.shouldContain, "expected Prometheus args to NOT contain %v, but got %v", test.expectedArg, promArgs)


I feel this is not a good method. There are many other places with same code. Wdyt?

What if we just use require.Equal(t, test.shouldContain, found)?

I think this could replace the whole if block 🤔

ArthurSens

There was a big refactoring of the tests in #6688, which already replaced the t.Fatal appearances. Could you rebase your branch and forget about the file pkg/prometheus/agent/statefulset_test.go? :)

I've done an incomplete review, I'll try to go over it again at another time!

ArthurSens · 2024-06-18T22:02:18Z

pkg/prometheus/agent/statefulset_test.go

-			if test.shouldContain {
-				t.Fatalf("expected Prometheus args to contain %v, but got %v", test.expectedArg, promArgs)
-			} else {
-				t.Fatalf("expected Prometheus args to NOT contain %v, but got %v", test.expectedArg, promArgs)
-			}
+			require.False(t, test.shouldContain, "expected Prometheus args to contain %v, but got %v", test.expectedArg, promArgs)
+			require.True(t, test.shouldContain, "expected Prometheus args to NOT contain %v, but got %v", test.expectedArg, promArgs)


What if we just use require.Equal(t, test.shouldContain, found)?

I think this could replace the whole if block 🤔

ArthurSens · 2024-06-18T22:14:34Z

pkg/prometheus/operator_test.go

+			if !test.expectErr {
+				require.NoError(t, err)


I think it looks good the way it is now, we just don't need the second if because, if we get there, it's always true.

What do you think about the suggestion below?

for _, c := range cases { test := c t.Run(test.name, func(t *testing.T) { err := ValidateRemoteWriteSpec(test.spec) if test.expectErr { require.Error(t, err) return } require.NoError(t, err) }) }

ArthurSens · 2024-06-18T22:16:57Z

pkg/prometheus/resource_selector_test.go

-			if err != nil && !tc.expectedErr {
-				t.Fatalf("expected no error, got: %v", err)
+			if !tc.expectedErr {
+				require.NoError(t, err)
 			}
-			if err == nil && tc.expectedErr {
-				t.Fatalf("expected an error, got nil")
+			if tc.expectedErr {
+				require.Error(t, err)


It's a similar situation here, we can use early returns to not need so many ifs

t.Run(tc.scenario, func(t *testing.T) { err := validateRelabelConfig(&tc.prometheus, tc.relabelConfig) if tc.expectedErr { require.Error(t, err) return } require.NoError(t, err) })

ArthurSens · 2024-06-18T22:17:40Z

pkg/prometheus/resource_selector_test.go

 				t.Logf("err %v", err)
-				if err != nil && !tc.expectedErr {
-					t.Fatalf("expected no error, got: %v", err)
+				if !tc.expectedErr {
+					require.NoError(t, err)
 				}
-				if err == nil && tc.expectedErr {
-					t.Fatalf("expected an error, got nil")
+				if tc.expectedErr {
+					require.Error(t, err)


Same thing here :)

ArthurSens · 2024-06-18T22:19:21Z

pkg/prometheus/server/rules_test.go

+	require.Error(t, err)
+	require.Equal(t, expectedError, err.Error(), "expected makeRulesConfigMaps to return error '%v' but got '%v'", expectedError, err)


Suggested change

require.Error(t, err)

require.Equal(t, expectedError, err.Error(), "expected makeRulesConfigMaps to return error '%v' but got '%v'", expectedError, err)

require.Equal(t, expectedError, err.Error(), "expected makeRulesConfigMaps to return error '%v' but got '%v'", expectedError, err)

If err is nil, the require.Equal(...) will catch it already, right?

AshwinSriram11 · 2024-06-28T13:21:25Z

pkg/prometheus/server/statefulset_test.go

-		if !found {
-			t.Fatalf("--tsdb.path argument should be given to the Thanos sidecar, got %q", strings.Join(sset.Spec.Template.Spec.Containers[3].Args, " "))
-		}
+		require.True(t, found, "--tsdb.path argument should be given to the Thanos sidecar, got %q", strings.Join(sset.Spec.Template.Spec.Containers[2].Args, " "))


I changed the index from 3 to 2 because I received the following error locally on running the test

--- FAIL: TestThanosObjectStorageFile (0.00s) panic: runtime error: index out of range [3] with length 3 [recovered] panic: runtime error: index out of range [3] with length 3

After changing, it was running fine.

AshwinSriram11 · 2024-06-28T13:22:39Z

pkg/prometheus/server/statefulset_test.go

-		if !found {
-			t.Fatalf("--tsdb.path argument should be given to the Thanos sidecar, got %q", strings.Join(sset.Spec.Template.Spec.Containers[3].Args, " "))
-		}
+		require.True(t, found, "--tsdb.path argument should be given to the Thanos sidecar, got %q", strings.Join(sset.Spec.Template.Spec.Containers[2].Args, " "))


Same here too

I changed the index from 3 to 2 because I received the following error locally on running the test

--- FAIL: TestThanosObjectStorageFile (0.00s) panic: runtime error: index out of range [3] with length 3 [recovered] panic: runtime error: index out of range [3] with length 3

After changing, it was running fine.

ArthurSens

I've added just a few nitpicks, but generally LGTM :)

ArthurSens · 2024-06-28T17:04:40Z

pkg/prometheus/agent/operator_test.go

-			t.Fatalf("Expected input %sto be matching a prometheus agent key, but did not", notExp)
-		}
+		require.Equal(t, c.expectedKey, key, "Expected prometheus agent key %q got %q", c.expectedKey, key)
+		require.Equal(t, c.expectedMatch, match, "Expected input %sto be matching a prometheus agent key, but did not", map[bool]string{true: "", false: "not "}[c.expectedMatch])


Suggested change

require.Equal(t, c.expectedMatch, match, "Expected input %sto be matching a prometheus agent key, but did not", map[bool]string{true: "", false: "not "}[c.expectedMatch])

require.Equal(t, c.expectedMatch, match)

I think this complicated logic doesn't help much 😅

ArthurSens · 2024-06-28T17:06:14Z

pkg/prometheus/server/operator_test.go

-			t.Fatalf("Expected input %sto be matching a prometheus key, but did not", notExp)
-		}
+		require.Equal(t, c.expectedKey, key, "Expected prometheus key %q got %q", c.expectedKey, key)
+		require.Equal(t, c.expectedMatch, match, "Expected input %sto be matching a prometheus key, but did not", map[bool]string{true: "", false: "not "}[c.expectedMatch])


Same here 😬

ArthurSens · 2024-06-28T17:10:29Z

pkg/prometheus/server/statefulset_test.go

+	valLabel, ok := sset.Spec.Template.ObjectMeta.Labels["testlabel"]
+	require.True(t, (ok && valLabel == "testvalue"), "Pod labels are not properly propagated")


Does it make sense to split this in 2 requires?

require.True(t, ok) require.Equal(t, valLabel, "testvalue")

We could even just use require.Equal(t, valLabel, "testvalue") and forget about asserting ok I think. Because if valLabel == "testvalue" it's impossible for ok to be false

Yeah, didn't think of it. Nice catch.

ArthurSens · 2024-06-28T17:10:46Z

pkg/prometheus/server/statefulset_test.go

+	valAnnotation, ok := sset.Spec.Template.ObjectMeta.Annotations["testannotation"]
+	require.True(t, (ok && valAnnotation == "testvalue"), "Pod annotations are not properly propagated")


ArthurSens · 2024-06-28T17:45:40Z

pkg/prometheus/server/statefulset_test.go

+		require.False(t, (foundRetention != test.shouldContainRetention || foundRetentionFlag != test.shouldContainRetention), "test %d, expected Prometheus args to %scontain %v, but got %v", i, map[bool]string{true: "", false: "NOT "}[test.shouldContainRetention], test.expectedRetentionArg, promArgs)
+		require.False(t, (foundRetentionSize != test.shouldContainRetentionSize || foundRetentionSizeFlag != test.shouldContainRetentionSize), "test %d, expected Prometheus args to %scontain %v, but got %v", i, map[bool]string{true: "", false: "NOT "}[test.shouldContainRetentionSize], test.expectedRetentionSizeArg, promArgs)


We don't necessarily need to keep the previous message with this refactoring. It would be even better to not add them at all I believe.

We added messages with t.Fatal because t.Fatal has no context whatsoever. The require package already adds a lot of context to the test, and emits nice human readable comparisons when the test fails.

We could simplify here with just:

if test.shouldContainRetention { require.True(t, (foundRetention && foundRetentionFlag)) } if test.shouldContainRetentionSize { require.True(t, (foundRetentionSize && foundRetentionSizeFlag)) }

ArthurSens

Good job, thanks!

AshwinSriram11 requested a review from a team as a code owner June 18, 2024 11:39

pull-request-size bot added the size/XL label Jun 18, 2024

AshwinSriram11 commented Jun 18, 2024

View reviewed changes

ArthurSens reviewed Jun 18, 2024

View reviewed changes

Fixes after rebase

509013b

AshwinSriram11 force-pushed the replace-Fatal-3 branch from 7e6342e to 509013b Compare June 28, 2024 13:07

Whitespace removed

5adf47a

AshwinSriram11 commented Jun 28, 2024

View reviewed changes

ArthurSens previously approved these changes Jun 28, 2024

View reviewed changes

Minor changes

2831f31

AshwinSriram11 dismissed ArthurSens’s stale review via 2831f31 June 29, 2024 01:57

ArthurSens approved these changes Jun 29, 2024

View reviewed changes

ArthurSens merged commit 709f4bf into prometheus-operator:main Jun 29, 2024
18 checks passed

AshwinSriram11 deleted the replace-Fatal-3 branch July 10, 2024 09:55

AshwinSriram11 mentioned this pull request Jul 10, 2024

Replace t.Fatal with require in pkg/thanos #6749

Merged

5 tasks

AshwinSriram11 mentioned this pull request Jul 30, 2024

Replace t.Fatal with require package in tests-e2e #6782

Merged

5 tasks

AshwinSriram11 mentioned this pull request Sep 19, 2024

Replace t.Fatal with require package #6952

Closed

5 tasks

		require.Error(t, err)
		require.Equal(t, expectedError, err.Error(), "expected makeRulesConfigMaps to return error '%v' but got '%v'", expectedError, err)

	require.Equal(t, c.expectedMatch, match, "Expected input %sto be matching a prometheus agent key, but did not", map[bool]string{true: "", false: "not "}[c.expectedMatch])
	require.Equal(t, c.expectedMatch, match)

		valLabel, ok := sset.Spec.Template.ObjectMeta.Labels["testlabel"]
		require.True(t, (ok && valLabel == "testvalue"), "Pod labels are not properly propagated")

		valAnnotation, ok := sset.Spec.Template.ObjectMeta.Annotations["testannotation"]
		require.True(t, (ok && valAnnotation == "testvalue"), "Pod annotations are not properly propagated")

		require.False(t, (foundRetention != test.shouldContainRetention \|\| foundRetentionFlag != test.shouldContainRetention), "test %d, expected Prometheus args to %scontain %v, but got %v", i, map[bool]string{true: "", false: "NOT "}[test.shouldContainRetention], test.expectedRetentionArg, promArgs)
		require.False(t, (foundRetentionSize != test.shouldContainRetentionSize \|\| foundRetentionSizeFlag != test.shouldContainRetentionSize), "test %d, expected Prometheus args to %scontain %v, but got %v", i, map[bool]string{true: "", false: "NOT "}[test.shouldContainRetentionSize], test.expectedRetentionSizeArg, promArgs)

Replace t.Fatal with require in Prometheus package #6690

Replace t.Fatal with require in Prometheus package #6690

Uh oh!

Conversation

AshwinSriram11 commented Jun 18, 2024

Description

Type of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArthurSens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!