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 <arihant0pal@gmail.com>

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

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

Ensure that the Pull Request has a descriptive title.
Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

Apply the release notes (needs details) label if users need to know about this change.
New features should be documented.
There should be some code comments as to why things are implemented the way they are.
There should be a comment at the top of each new or modified test to explain what the test does.

New flags

Is this flag really necessary?
Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

Each item in Jobs should be named in order to mark it as required.
If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

Protobuf changes should be wire-compatible.
Changes to _vt tables and RPCs need to be backward compatible.
RPC changes should be compatible with vitess-operator
If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
vtctl command output order should be stable and awk-able.

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

Update_2

91d9c25

Signed-off-by: Ari1009 <arihant0pal@gmail.com>

shlomi-noach requested changes Jun 3, 2024

View reviewed changes

Updates_3

4239566

Signed-off-by: Ari1009 <arihant0pal@gmail.com>

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 <arihant0pal@gmail.com>

Ari1009 added 2 commits June 4, 2024 22:52

Remove_UnecessaryPrint

6b4b485

Signed-off-by: Ari1009 <arihant0pal@gmail.com>

cleanup

e03a735

Signed-off-by: Ari1009 <arihant0pal@gmail.com>

dbussink reviewed Jun 4, 2024

View reviewed changes

Cleanup_2

49876f3

Signed-off-by: Ari1009 <arihant0pal@gmail.com>

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