Refactor: remove key parameter from testvar methods #7040
Conversation
alexshtin
force-pushed
the
feature/refactor-testvars
branch
3 times, most recently
from
318b87f to
499f31c
Compare
alexshtin
force-pushed
the
feature/refactor-testvars
branch
from
499f31c to
98c8720
Compare
alexshtin
force-pushed
the
feature/refactor-testvars
branch
from
98c8720 to
6689596
Compare
| @@ -1279,7 +1271,7 @@ func makeCurrentWorkflowConditionFailedError( | |||
| lastWriteVersion := common.EmptyVersion | |||
| return &persistence.CurrentWorkflowConditionFailedError{ | |||
| Msg: "random message", | |||
| RequestID: tv.String("PrevRequestID"), | |||
| RequestID: tv.RequestID(), | |||
There was a problem hiding this comment.
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"?
| 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()) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
limit is defined in another test, as internal variable, and used in a closure.
this was super hard to read.
| @@ -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") | |||
There was a problem hiding this comment.
and here for some reason you are not using tv1.
even though you are replacing the same code.
There was a problem hiding this comment.
and same thoughts - tv.At("update_id_1") reads better.
| @@ -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", | |||
There was a problem hiding this comment.
same here, why you are not using:
tv1 := tv.WithUpdateID("1")
?
| tv := testvars.New(s.T()). | ||
| WithTaskQueue(s.TaskQueue()). | ||
| WithNamespaceName(namespace.Name(s.Namespace())). | ||
| WithRunID("") |
There was a problem hiding this comment.
this basically "reset run ID".
Why?
| @@ -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()). | |||
| @@ -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 | |||
There was a problem hiding this comment.
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?
| @@ -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", | |||
There was a problem hiding this comment.
shouldn't it be tv.MessageID()+"_update-accepted_1"?
| func (tv *TestVars) Entity() string { | ||
| return getOrCreate(tv, "entity", tv.uniqueString) | ||
| } | ||
| func (tv *TestVars) WithEntity(entity string) *TestVars { |
There was a problem hiding this comment.
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(), ...)
There was a problem hiding this comment.
those are main comments, rest are oservations.
What changed?
Remove
keyparameter fromtestvarmethods.Why?
It turn out to be confusing for people on how to use optional
keyparameter. @bergundy got a great idea to remove this parameter and use different instance oftvwhen 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.