[#180] node: Refactor panics in unit test #182

aarifullin · 2023-03-28T14:20:49Z

aarifullin commented

2023-03-28 14:20:49 +00:00

Replace panics in unit tests by require.NoError and t.Fatalf

Signed-off-by: Airat Arifullin a.arifullin@yadro.com

* Replace panics in unit tests by require.NoError and t.Fatalf Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

aarifullin added the

frostfs-node

refactoring

labels 2023-03-28 14:20:50 +00:00

aarifullin requested review from storage-core-committers 2023-03-28 14:21:36 +00:00

aarifullin added a new dependency 2023-03-28 14:29:25 +00:00

#180 Make unit tests use require.NoError/t.Fatalf instead panic

aarifullin removed a dependency 2023-03-28 14:29:37 +00:00

#180 Make unit tests use require.NoError/t.Fatalf instead panic

fyrchik reviewed 2023-03-28 16:43:33 +00:00

fyrchik left a comment

Overall LGTM, but I think we could still replace some 3-line ifs with a single require

Overall LGTM, but I think we could still replace some 3-line ifs with a single `require`

pkg/local_object_storage/engine/evacuate_test.go Outdated

					
				@ -157,3 +157,3 @@

							}

							require.FailNow(t, "handler was called with an unexpected object: %s", addr)

							panic("unreachable")

							return errors.New("unreachable")

fyrchik commented

2023-03-28 16:41:14 +00:00

IMO panic better communicate our intentions here.

👍 1

aarifullin marked this conversation as resolved

pkg/local_object_storage/internal/testutil/generators.go Outdated

					
				@ -100,3 +101,3 @@

						addr.SetObject(id)

					} else {

						panic("object ID is not set")

						t.Fatalf("object ID is not set")

fyrchik commented

2023-03-28 16:42:15 +00:00

Can we remove an if then?

id, isSet := obj.ID()
require.True(t, isSet, "object ID is not set")

addr.SetObject(id)

Can we remove an if then? ``` id, isSet := obj.ID() require.True(t, isSet, "object ID is not set") addr.SetObject(id) ```

👍 1

aarifullin marked this conversation as resolved

pkg/util/logger/test/logger.go Outdated

					
				@ -27,3 +29,3 @@

						log, err := cfg.Build()

						if err != nil {

							panic("could not prepare logger: " + err.Error())

							t.Fatalf("could not prepare logger: %v", err)

fyrchik commented

2023-03-28 16:42:55 +00:00

require.NoError?

`require.NoError`?

👍 1

aarifullin marked this conversation as resolved

aarifullin force-pushed feature/180-factor_out_panics from 4c34e924df to 7156e0b397

2023-03-28 20:12:17 +00:00

Compare

aarifullin force-pushed feature/180-factor_out_panics from 7156e0b397 to e9dc4d43ef

2023-03-28 20:14:37 +00:00

Compare

ale64bit requested changes 2023-03-29 07:15:02 +00:00

pkg/services/tree/getsubtree_test.go Outdated

					
				@ -120,2 +120,3 @@

				var errSubTreeSend = errors.New("test error")

				var (

					errSubTreeSend    = errors.New("test error")

ale64bit commented

2023-03-29 07:14:14 +00:00

even if it's a test error, can we use a more meaningful message?

👍 1

aarifullin commented

2023-03-29 07:36:30 +00:00

Done

ale64bit marked this conversation as resolved

aarifullin requested review from storage-core-developers 2023-03-29 07:17:24 +00:00

aarifullin force-pushed feature/180-factor_out_panics from e9dc4d43ef to 6786dcf950

2023-03-29 07:35:49 +00:00

Compare

dstepanov-yadro requested changes 2023-03-29 07:49:23 +00:00

pkg/services/tree/getsubtree_test.go Outdated

					
				@ -120,2 +120,3 @@

				var errSubTreeSend = errors.New("test error")

				var (

					errSubTreeSend           = errors.New("Send finished with error")

dstepanov-yadro commented

2023-03-29 07:49:20 +00:00

In general error strings should not be capitalized. From https://github.com/golang/go/wiki/CodeReviewComments#error-strings

As far as I understand, we follow this rule.

*In general error strings should not be capitalized*. From https://github.com/golang/go/wiki/CodeReviewComments#error-strings As far as I understand, we follow this rule.

aarifullin commented

2023-03-29 09:38:08 +00:00

Yes, it's my fault. Here Send is name of the method. Just coincidence :) I'll fix it anyway