test: Replace t.fatalf with testify require #16038

Ari1009 · 2024-06-02T08:16:37Z

Description

This PR replaces some t.fatalf instances with testify's require

Related Issue(s)

#15182

Checklist

"Backport to:" labels have been added if this change should be back-ported to release branches
If this change is to be back-ported to previous releases, a justification is included in the PR description
Tests were added or are not required
Did the new or modified tests pass consistently locally and on CI?
Documentation was added or is not required

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

vitess-bot · 2024-06-02T08:16:40Z

shlomi-noach

Thank you for this cleanup PR! While we're doing this, let's do it in the most idiomatic way. Please see inline comments for suggestions and issues!

shlomi-noach · 2024-06-02T10:39:23Z

go/vt/binlog/binlog_streamer_rbr_test.go

-	if err != ErrServerEOF {
-		t.Errorf("unexpected error: %v", err)
-	}
+	require.Equal(t, ErrServerEOF, err, "unexpected error: %v", err)


Should we use EqualError?

shlomi-noach · 2024-06-02T10:40:08Z

go/tools/asthelpergen/asthelpergen_test.go

-		if applyIdx == 0 && cloneIdx == 0 {
-			t.Fatalf("file doesn't contain expected contents")
-		}
+		assert.False(t, applyIdx == 0 && cloneIdx == 0, "file doesn't contain expected contents")


Seems like a redaction of severity from Fatalf to assert. Should this not be at least require?

shlomi-noach · 2024-06-02T10:41:53Z

go/vt/binlog/binlog_streamer_rbr_test.go

-		for i, fbt := range want {
-			t.Errorf("Want(%v)=%v", i, fbt.statements)
-		}
+	assert.True(t, reflect.DeepEqual(got, want), "binlogConnStreamer.parseEvents(): got:\n%+v\nwant:\n%+v", got, want)


assert.Equal uses reflect.DeepEqual under the hood. We should just use assert.Equal(t, want, got).

shlomi-noach · 2024-06-02T10:43:41Z

go/vt/binlog/binlog_streamer_rbr_test.go

+	for i, fbt := range got {
+		assert.Equal(t, fbt.statements, want[i].statements, "Got (%v)=%v, want (%v)=%v", i, fbt.statements, i, want[i].statements)


This seems superfluous given we've validated the equality of the entire slice, above?

shlomi-noach · 2024-06-02T10:44:38Z

go/vt/binlog/tables_filter_test.go

-	if want != got {
-		t.Errorf("want\n%s, got\n%s", want, got)
-	}
+	assert.Equal(t, want, got, "binlogConnStreamer.tablesFilterFunc(): got:\n%+v\nwant:\n%+v", got, want)


We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

shlomi-noach · 2024-06-02T10:47:35Z

go/vt/wrangler/tablet_test.go

+	require.Error(t, err, "DeleteTablet on primary: want error")
+	require.Contains(t, err.Error(), wantError, "DeleteTablet on primary: want specific error message")


Let's use a single ErrorContains test instead of the two tests above.

shlomi-noach · 2024-06-02T10:48:24Z

go/vt/wrangler/vexec_test.go

-					if !testCase.result.Equal(result) {
-						t.Errorf("mismatched result:\nwant: %v\ngot:  %v", testCase.result, result)
-					}
+					assert.True(t, testCase.result.Equal(result), "mismatched result:\nwant: %v\ngot:  %v", testCase.result, result)


We should remove the got:\n%+v\nwant:\n%+v output, since testify prints the same kind of output anyway.

shlomi-noach · 2024-06-02T10:48:46Z

go/vt/wrangler/vexec_test.go

-				if !strings.Contains(err.Error(), testCase.errorString) {
-					t.Fatalf("Wrong error, want %s, got %s", testCase.errorString, err.Error())
-				}
+				assert.Contains(t, err.Error(), testCase.errorString, "Wrong error, want %s, got %s", testCase.errorString, err.Error())


Let's use ErrorContains() instead.

shlomi-noach · 2024-06-02T10:49:24Z

go/vt/zkctl/zkctl_test.go

-	}
+	zkObservedConf, err := MakeZooCfg([]string{zkConf.ConfigFile()}, zkConf, "header")
+	require.NoError(t, err, "MakeZooCfg err: %v", err)
+	assert.Contains(t, zkObservedConf, fmt.Sprintf("\n%s\n", tpcKeepAliveCfg), "Expected tpcKeepAliveCfg in zkObservedConf")


Should be require given the original Fatalf?

shlomi-noach · 2024-06-02T10:49:48Z

go/vt/zkctl/zkctl_test.go

+	zkObservedConf, err := MakeZooCfg([]string{zkConf.ConfigFile()}, zkConf, "header")
+	require.NoError(t, err, "MakeZooCfg err: %v", err)
+	assert.Contains(t, zkObservedConf, fmt.Sprintf("\n%s\n", tpcKeepAliveCfg), "Expected tpcKeepAliveCfg in zkObservedConf")
+	assert.Contains(t, zkObservedConf, fmt.Sprintf("\n%s\n", adminServerCfg), "Expected adminServerCfg in zkObservedConf")


should be require?

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

Ari1009 · 2024-06-03T07:04:53Z

Thanks, @shlomi-noach, for the review. I have made the necessary changes. Could you please check it out?

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

codecov · 2024-06-03T07:47:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.23%. Comparing base (b49494e) to head (ccdc5f1).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #16038   +/-   ##
=======================================
  Coverage   68.23%   68.23%           
=======================================
  Files        1543     1543           
  Lines      197555   197555           
=======================================
+ Hits       134796   134808   +12     
+ Misses      62759    62747   -12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shlomi-noach · 2024-06-03T09:50:02Z

Static code checks fails on:

Run out=$(goimports -local vitess.io/vitess -l -w $(find . -name "*.go" | grep -v ".pb.go"))
The following files are malformatted:
./go/vt/wrangler/tablet_test.go
./go/vt/binlog/binlog_streamer_rbr_test.go
./go/vt/binlog/tables_filter_test.go

Can you please format those files via goimports?

shlomi-noach

A couple more changes needed, as well as formatting. Thank you!

shlomi-noach · 2024-06-03T09:52:05Z

go/trace/trace_test.go

 	}

 	if tracer.name != serviceName {
-		t.Fatalf("expected the name to be `%v` but it was `%v`", serviceName, tracer.name)
+		require.FailNow(t, fmt.Sprintf("expected the name to be `%v` but it was `%v`", serviceName, tracer.name))


Shall we replace with require.Equal(t, serviceName, tracer.name)?

shlomi-noach · 2024-06-03T09:52:31Z

go/trace/trace_test.go

@@ -62,11 +62,11 @@ func TestRegisterService(t *testing.T) {
 	closer := StartTracing(serviceName)
 	tracer, ok := closer.(*fakeTracer)
 	if !ok {
-		t.Fatalf("did not get the expected tracer, got %+v (%T)", tracer, tracer)
+		require.FailNow(t, fmt.Sprintf("did not get the expected tracer, got %+v (%T)", tracer, tracer))


shall we replace this if statement with require.True(t, ok, ...)?

go/vt/binlog/binlog_streamer_rbr_test.go

shlomi-noach · 2024-06-03T09:54:03Z

go/vt/binlog/binlog_streamer_rbr_test.go

-		}
-	}
+	assert.EqualError(t, err, ErrServerEOF.Error(), "unexpected error")
+	assert.Equal(t, want, got, "binlogConnStreamer.parseEvents(): got:\n%+v\nwant:\n%+v", got, want)


let's get rid of got:\n%+v\nwant:\n%+v

shlomi-noach · 2024-06-03T09:54:35Z

go/vt/binlog/binlog_streamer_rbr_test.go

-		}
-	}
+	assert.EqualError(t, err, ErrServerEOF.Error(), "unexpected error")
+	assert.Equal(t, want, got, "binlogConnStreamer.parseEvents(): got:\n%+v\nwant:\n%+v", got, want)


let's get rid of got:\n%+v\nwant:\n%+v

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

Ari1009 · 2024-06-03T11:55:05Z

@shlomi-noach made changes. Thanks for the quick updates.

go/tools/asthelpergen/asthelpergen_test.go

go/trace/trace_test.go

go/vt/wrangler/tablet_test.go

go/vt/zkctl/zkctl_test.go

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

Ari1009 · 2024-06-04T14:58:48Z

@dbussink , Just made those changes you mentioned. ‍ Sorry about the weird formatting with the license. Seems an extension I had messed things up when saving. My bad!

dbussink · 2024-06-04T15:05:11Z

@dbussink , Just made those changes you mentioned. ‍ Sorry about the weird formatting with the license. Seems an extension I had messed things up when saving. My bad!

Commented, but there's many other cases of require.NoError(t, err) that can be cleaned up and don't really need any context.

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

Ari1009 · 2024-06-04T18:38:42Z

@dbussink, I removed strings that didn't seem to have any relevant context. Are there any other strings you think might be unnecessary?
There is:
GetTablet failed
DeleteTablet failed
UpdateShardFields failed
DeleteShard failed

dbussink · 2024-06-04T19:50:25Z

go/vt/wrangler/tablet_test.go

-		t.Fatalf("DeleteTablet failed: %v", err)
-	}
+	err = wr.DeleteTablet(context.Background(), tablet.Alias, true)
+	require.NoError(t, err, "DeleteTablet failed")


@Ari1009 I think you can remove this one.

go/vt/wrangler/tablet_test.go

dbussink · 2024-06-04T19:51:06Z

go/vt/wrangler/tablet_test.go

-		t.Fatalf("UpdateShardFields failed: %v", err)
-	}
+	})
+	require.NoError(t, err, "UpdateShardFields failed")


No need for the last string here either.

dbussink · 2024-06-04T19:51:22Z

go/vt/wrangler/tablet_test.go

-		t.Fatalf("UpdateShardFields failed: %v", err)
-	}
+	})
+	require.NoError(t, err, "UpdateShardFields failed")


Here as well.

dbussink · 2024-06-04T19:51:28Z

go/vt/wrangler/tablet_test.go

-		t.Fatalf("DeleteTablet failed: %v", err)
-	}
+	err = wr.DeleteTablet(context.Background(), tablet1.Alias, false)
+	require.NoError(t, err, "DeleteTablet failed")


dbussink · 2024-06-04T19:52:15Z

go/vt/zkctl/zkctl_test.go

-		t.Fatalf("Expected adminServerCfg in zkObservedConf")
-	}
+	zkObservedConf, err := MakeZooCfg([]string{zkConf.ConfigFile()}, zkConf, "header")
+	require.NoError(t, err, "MakeZooCfg err: %v", err)


This doesn't seem useful to print the same error again.

dbussink · 2024-06-04T19:52:26Z

go/vt/zkctl/zkctl_test.go

-		t.Fatalf("Init() err: %v", err)
-	}
+	err = zkd.Init()
+	require.NoError(t, err, "Init() err: %v", err)


Same here, not very useful to print the same error again.

dbussink · 2024-06-04T19:52:34Z

go/vt/zkctl/zkctl_test.go

-		t.Fatalf("Shutdown() err: %v", err)
-	}
+	err = zkd.Shutdown()
+	require.NoError(t, err, "Shutdown() err: %v", err)


No need here either.

dbussink · 2024-06-04T19:52:39Z

go/vt/zkctl/zkctl_test.go

-		t.Fatalf("Teardown() err: %v", err)
-	}
+	err = zkd.Start()
+	require.NoError(t, err, "Start() err: %v", err)


No need here either.

dbussink · 2024-06-04T19:52:43Z

go/vt/zkctl/zkctl_test.go


+	err = zkd.Teardown()
+	require.NoError(t, err, "Teardown() err: %v", err)


No need here either.

dbussink · 2024-06-04T19:54:05Z

@dbussink, I removed strings that didn't seem to have any relevant context. Are there any other strings you think might be unnecessary?

I don't think it's needed anywhere, except if other / additional information is provided beyond the actual error. But I don't think that's anywhere here, so we don't really need it. Just a plain string isn't that useful either, it already points at the line where it fails so it's obvious then which operation failed, no need to repeat that in the string.

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

Ari1009 · 2024-06-05T05:25:35Z

@dbussink, I have completed the necessary cleanups.

systay · 2024-06-14T13:14:00Z

Thank you for your contribution, @Ari1009!

fix

e0eee4f

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

Ari1009 requested review from deepthi, mattlord, rohit-nayak-ps, frouioui and systay as code owners June 2, 2024 08:16

github-actions bot added this to the v20.0.0 milestone Jun 2, 2024

shlomi-noach requested changes Jun 2, 2024

View reviewed changes

update

6b67e76

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

Update_2

91d9c25

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

shlomi-noach requested changes Jun 3, 2024

View reviewed changes

Updates_3

4239566

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

shlomi-noach approved these changes Jun 3, 2024

View reviewed changes

dbussink reviewed Jun 4, 2024

View reviewed changes

Update_4

3c10f00

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

Ari1009 added 2 commits June 4, 2024 22:52

Remove_UnecessaryPrint

6b4b485

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

cleanup

e03a735

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

dbussink reviewed Jun 4, 2024

View reviewed changes

Cleanup_2

49876f3

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

Merge branch 'vitessio:main' into fix

ccdc5f1

systay approved these changes Jun 14, 2024

View reviewed changes

systay merged commit 9d63ab4 into vitessio:main Jun 14, 2024
95 checks passed

Ari1009 deleted the fix branch June 29, 2024 11:56

		for i, fbt := range got {
		assert.Equal(t, fbt.statements, want[i].statements, "Got (%v)=%v, want (%v)=%v", i, fbt.statements, i, want[i].statements)

		require.Error(t, err, "DeleteTablet on primary: want error")
		require.Contains(t, err.Error(), wantError, "DeleteTablet on primary: want specific error message")


		err = zkd.Teardown()
		require.NoError(t, err, "Teardown() err: %v", err)

test: Replace t.fatalf with testify require #16038

test: Replace t.fatalf with testify require #16038

Conversation

Ari1009 commented Jun 2, 2024 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Jun 2, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ari1009 commented Jun 3, 2024

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

shlomi-noach commented Jun 3, 2024

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ari1009 commented Jun 3, 2024

Ari1009 commented Jun 4, 2024 • edited Loading

dbussink commented Jun 4, 2024

Ari1009 commented Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbussink commented Jun 4, 2024

Ari1009 commented Jun 5, 2024

systay commented Jun 14, 2024

Ari1009 commented Jun 2, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

Ari1009 commented Jun 4, 2024 •

edited

Loading

Ari1009 commented Jun 4, 2024 •

edited

Loading