-
Notifications
You must be signed in to change notification settings - Fork 801
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds test for execution/mutable_state_builder.go #5744
Adds test for execution/mutable_state_builder.go #5744
Conversation
Codecov Report
Additional details and impacted files
... and 14 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018e16d6-0f2b-4f72-8c31-01dd993f21b8Details
💛 - Coveralls |
@@ -990,3 +994,274 @@ func (s *mutableStateSuite) buildWorkflowMutableState() *persistence.WorkflowMut | |||
VersionHistories: versionHistories, | |||
} | |||
} | |||
|
|||
func TestNewMutableStateBuilderWithEventV2(t *testing.T) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove empty first lines in functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but I'd suggest putting that in lint if it's a thing we want to do more generally
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to remove generous new lines like this via linter which IMO hurts readability a little bit. Consistently spaced codebase is much easier to read thru. We are far from consistency so this new line is not in the top of my list :) Maybe @Groxx can quickly come up with the magic regex for this too
}, nil) | ||
|
||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove empty line
|
||
for name, td := range tests { | ||
t.Run(name, func(t *testing.T) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove empty first line of funcs
res, err := td.currentState.GetCompletionEvent(context.Background()) | ||
|
||
assert.Equal(t, td.expectedResult, res) | ||
assert.Equal(t, td.expectedErr, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.EqualError
does better job (I could be wrong. little experience with this assert package)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also admit not being super familiar with that specifically. It looks like it is just doing string comparison though so probably assert.ErrorAs
and assert.ErrorIs
I suspect. Let me look
This is part of a wider initiative to increase unit-test coverage for this package