Refactor: remove key parameter from testvar methods #7040

alexshtin · 2024-12-31T01:44:02Z

What changed?

Remove key parameter from testvar methods.

Why?

It turn out to be confusing for people on how to use optional key parameter. @bergundy got a great idea to remove this parameter and use different instance of tv when more than one entity is needed.

How did you test it?

Run all tests. Refactoring affects test code only.

Potential risks

No risks.

Documentation

Yes, updated testing docs.

Is hotfix candidate?

No.

ychebotarev · 2024-12-31T20:49:23Z

service/history/history_engine2_test.go

@@ -1279,7 +1271,7 @@ func makeCurrentWorkflowConditionFailedError(
 	lastWriteVersion := common.EmptyVersion
 	return &persistence.CurrentWorkflowConditionFailedError{
 		Msg:              "random message",
-		RequestID:        tv.String("PrevRequestID"),
+		RequestID:        tv.RequestID(),


I think it doesn't worth to wrap every parameter into tv function.
For example this code
startRequest.StartRequest.RequestId = tv.At("PrevRequestID")
lets me know that I'm re-using prev request ID.
Intentions are more clear.
tv.RequestID() leaves me with the question "why this will fail/not fail"?

Well, I think it does. So I removed that String() method at all. Let's create methods for every entity type.

ychebotarev · 2024-12-31T20:55:19Z

service/history/workflow/update/registry_test.go

 		require.False(t, existed)
 		require.Equal(t, 1, reg.Len())

 		t.Run("deny new update since it is exceeding the limit", func(t *testing.T) {
-			_, _, err = reg.FindOrCreate(context.Background(), tv.UpdateID("2"))
+			_, _, err = reg.FindOrCreate(context.Background(), tv2.UpdateID())


When reading this code it is very hard to instantly grasp why this should be denied.

_, _, err = reg.FindOrCreate(context.Background(), "second_update_id") is easier to understand.
or
tv.At("second_update_id") if we want to provide TV guaranties.

with the new approach, it will be more readable as:
_, _, err = reg.FindOrCreate(context.Background(), tv.WithUpdateID("second_update"))

Sorry, I don't understand how new version is harder to understand than previous one.

ychebotarev · 2024-12-31T21:00:54Z

service/history/workflow/update/registry_test.go

 			var resExh *serviceerror.ResourceExhausted
 			require.ErrorAs(t, err, &resExh, "creating update #2 should be denied")
 			require.Equal(t, 1, reg.Len())
 		})

 		t.Run("increasing limit allows new updated to be created", func(t *testing.T) {
-			_, _, err = reg.FindOrCreate(context.Background(), tv.UpdateID("2"))
+			_, _, err = reg.FindOrCreate(context.Background(), tv2.UpdateID())
 			var resExh *serviceerror.ResourceExhausted
 			require.ErrorAs(t, err, &resExh)
 			require.Equal(t, 1, reg.Len())

 			limit += 1


limit is defined in another test, as internal variable, and used in a closure.
this was super hard to read.

ychebotarev · 2024-12-31T21:06:38Z

service/history/workflow/update/registry_test.go

@@ -458,9 +461,9 @@ func TestSendMessages(t *testing.T) {

 	t.Run("registry with 2 created updates has no messages to send", func(t *testing.T) {
 		var err error
-		upd1, _, err = reg.FindOrCreate(context.Background(), tv.UpdateID("1"))
+		upd1, _, err = reg.FindOrCreate(context.Background(), tv.UpdateID()+"_1")


and here for some reason you are not using tv1.
even though you are replacing the same code.

and same thoughts - tv.At("update_id_1") reads better.

I changed all cases like this to use tv.WithUpdateIDN(1).UpdateID(). I agree it is a better, uniform way.

ychebotarev · 2024-12-31T21:07:22Z

service/history/workflow/update/registry_test.go

@@ -589,14 +592,14 @@ func TestAbort(t *testing.T) {
 	reg := update.NewRegistry(&mockUpdateStore{
 		VisitUpdatesFunc: func(visitor func(updID string, updInfo *persistencespb.UpdateInfo)) {
 			visitor(
-				tv.UpdateID("1"),
+				tv.UpdateID()+"_1",


same here, why you are not using:
tv1 := tv.WithUpdateID("1")
?

ychebotarev · 2024-12-31T22:32:21Z

tests/update_workflow_sdk_test.go

+	tv := testvars.New(s.T()).
+		WithTaskQueue(s.TaskQueue()).
+		WithNamespaceName(namespace.Name(s.Namespace())).
+		WithRunID("")


this basically "reset run ID".
Why?

Because default behavior is to generate random one, but in these tests I need WorkflowExecution() with empty RunID().

ychebotarev · 2024-12-31T22:32:49Z

tests/update_workflow_sdk_test.go

@@ -97,7 +100,10 @@ WorkflowExecutionTerminated // This can be EventID=3 if WF is terminated before
 func (s *UpdateWorkflowSdkSuite) TestUpdateWorkflow_TimeoutWorkflowAfterUpdateAccepted() {
 	ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
 	defer cancel()
-	tv := testvars.New(s.T()).WithTaskQueue(s.TaskQueue()).WithNamespaceName(namespace.Name(s.Namespace()))
+	tv := testvars.New(s.T()).


same question

ychebotarev · 2024-12-31T22:37:17Z

tests/update_workflow_test.go

@@ -166,7 +166,7 @@ func (s *UpdateWorkflowSuite) TestUpdateWorkflow_EmptySpeculativeWorkflowTask_Ac
  5 WorkflowTaskScheduled // Speculative WT events are not written to the history yet.
  6 WorkflowTaskStarted
 `, task.History)
-					return s.UpdateAcceptCompleteCommands(tv, "1"), nil
+					return s.UpdateAcceptCompleteCommands(tv), nil


this "1" means that all messages has "1". You removed it, but I don't see any WithUpdateId above. So UpdateID() will be without "1". Is that ok? If so - what was the purpose of "1" suffix before?

Well... this is a primary goal for entire PR. "1" is now encapsulated in tv.

ychebotarev · 2024-12-31T22:38:29Z

tests/update_workflow_test.go

@@ -1131,7 +1131,7 @@ func (s *UpdateWorkflowSuite) TestUpdateWorkflow_ValidateWorkerMessages() {
 				updRequest := protoutils.UnmarshalAny[*updatepb.Request](s.T(), reqMsg.GetBody())
 				return []*protocolpb.Message{
 					{
-						Id:                 tv.MessageID("update-accepted", "1"),
+						Id:                 tv.MessageID() + "_1",


shouldn't it be tv.MessageID()+"_update-accepted_1"?

ychebotarev · 2024-12-31T22:57:56Z

common/testing/testvars/test_vars.go

+func (tv *TestVars) Entity() string {
+	return getOrCreate(tv, "entity", tv.uniqueString)
+}
+func (tv *TestVars) WithEntity(entity string) *TestVars {


usually "With" semantic means you are operating within the same object, rather then creating new object. It is kind of part of the constructor ("chaining" pattern).
Code should look like
tv:=testvars.New(testvars.WithWorkflowId(), testvars.WithWorkflowId(), ...)

those are main comments, rest are oservations.

Using options pattern seems like an overkill here for me. This is a simple test helper which needs to be simple to use. Micro performance and memory allocations optimization is not important here.
As for naming convention, I believe, it is exactly the opposite. With indicates that you are getting new object "with specific property set to value". Otherwise it would be in go setter format: tv.UpdateID("new_update_id").

bergundy

I'll defer to Yuri here, the general direction LGTM.

ychebotarev · 2025-01-08T05:32:04Z

service/history/workflow/update/registry_test.go

 		require.False(t, existed)
 		require.Equal(t, 1, reg.Len())

 		t.Run("deny new update since it is exceeding the limit", func(t *testing.T) {
-			_, _, err = reg.FindOrCreate(context.Background(), tv.UpdateID("2"))
+			_, _, err = reg.FindOrCreate(context.Background(), tv2.UpdateID())


with the new approach, it will be more readable as:
_, _, err = reg.FindOrCreate(context.Background(), tv.WithUpdateID("second_update"))

ychebotarev · 2025-01-08T05:32:20Z

common/testing/testvars/test_vars.go

-func (tv *TestVars) BuildId(key ...string) string {
-	//revive:disable-next-line:unchecked-type-assertion
-	return tv.getOrCreate("build_id", key).(string)
+func (tv *TestVars) WithWorkflowIDN(n int) *TestVars {


What this N means at the end?

Why input parameter is int, and WorkflowID() return parameter is string? Why I can't add arbitrary string?

Why behavior of WithRunID is different from WithWorkflowIDN? (in WithRunID you call cloneSetVal, int WithWorkflowIDN you call cloneSetN)?

Why WithRunID accepting string, but WithWorkflowIDN accepting int?

This is very inconsistent and hard to understand.

I would (pretty strongly) suggest to

remove N

make all input parameters for WithXXXX strings instead of int if XXXX return string

use the same approach for every WithXXX functions (either cloneSetN, or cloneSetVal)

ychebotarev · 2025-01-08T05:35:37Z

common/testing/testvars/test_vars.go

 }

-func (tv *TestVars) RunID(key ...string) string {
-	return tv.getOrCreate("run_id", key, "").(string)
+func (tv *TestVars) WithBuildID(buildId string) *TestVars {


tv.WithBuildID("1").BuildID() != tv.WithBuildIDN(1).BuildID()

I don't think this is predictable and intuitive, I wouldn't expect that.
What is the reason for that? Why we have 2 such function with different behaviour?

alexshtin requested a review from a team as a code owner December 31, 2024 01:44

alexshtin force-pushed the feature/refactor-testvars branch 3 times, most recently from 318b87f to 499f31c Compare December 31, 2024 06:22

alexshtin requested review from bergundy and ychebotarev December 31, 2024 20:04

alexshtin force-pushed the feature/refactor-testvars branch 2 times, most recently from 98c8720 to 6689596 Compare December 31, 2024 20:33

ychebotarev reviewed Dec 31, 2024

View reviewed changes

bergundy reviewed Jan 7, 2025

View reviewed changes

alexshtin force-pushed the feature/refactor-testvars branch from 6689596 to 4dcfe71 Compare January 8, 2025 01:34

ychebotarev reviewed Jan 8, 2025

View reviewed changes

alexshtin requested a review from ychebotarev January 9, 2025 00:01

alexshtin added 6 commits January 8, 2025 18:21

Refactor: remove key parameter from testvar methods

403ee34

Change AppendToEntity to WithEntityN

74f5319

Add N on the fly

5a67710

Fix doc

9a95e89

Fix lint

5d5b35c

Minor improvement

8dae771

alexshtin force-pushed the feature/refactor-testvars branch from 84d4e62 to 8dae771 Compare January 9, 2025 02:22

alexshtin added 2 commits January 8, 2025 22:39

Rename N to Number

eb0eb51

Fix versioning tests

a5a3022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: remove key parameter from testvar methods #7040

Refactor: remove key parameter from testvar methods #7040

alexshtin commented Dec 31, 2024 •

edited

Loading

ychebotarev Dec 31, 2024

alexshtin Jan 8, 2025

ychebotarev Dec 31, 2024

ychebotarev Jan 8, 2025

alexshtin Jan 8, 2025

ychebotarev Dec 31, 2024

ychebotarev Dec 31, 2024

ychebotarev Dec 31, 2024

alexshtin Jan 8, 2025

ychebotarev Dec 31, 2024

ychebotarev Dec 31, 2024

alexshtin Jan 8, 2025

ychebotarev Dec 31, 2024

ychebotarev Dec 31, 2024

alexshtin Jan 8, 2025

ychebotarev Dec 31, 2024

ychebotarev Dec 31, 2024

ychebotarev Dec 31, 2024

alexshtin Jan 8, 2025

bergundy left a comment

ychebotarev Jan 8, 2025

ychebotarev Jan 8, 2025

ychebotarev Jan 8, 2025 •

edited

Loading

Refactor: remove key parameter from testvar methods #7040

Are you sure you want to change the base?

Refactor: remove key parameter from testvar methods #7040

Conversation

alexshtin commented Dec 31, 2024 • edited Loading

What changed?

Why?

How did you test it?

Potential risks

Documentation

Is hotfix candidate?

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

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

bergundy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ychebotarev Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

alexshtin commented Dec 31, 2024 •

edited

Loading

ychebotarev Jan 8, 2025 •

edited

Loading