Skip to content
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

Conversation

davidporter-id-au
Copy link
Contributor

@davidporter-id-au davidporter-id-au commented Mar 6, 2024

  • Adds tests for GetCompletionEvent, GetStartEvent.
  • Adds a mock for shard.Context

This is part of a wider initiative to increase unit-test coverage for this package

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Merging #5744 (fe62c70) into master (ea4bc19) will decrease coverage by 0.32%.
The diff coverage is n/a.

❗ Current head fe62c70 differs from pull request most recent head 5eedf7d. Consider uploading reports for the commit 5eedf7d to get more accurate results

Additional details and impacted files
Files Coverage Δ
service/history/shard/context.go 30.22% <ø> (ø)

... and 14 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea4bc19...5eedf7d. Read the comment docs.

@coveralls
Copy link

coveralls commented Mar 6, 2024

Pull Request Test Coverage Report for Build 018e16d6-0f2b-4f72-8c31-01dd993f21b8

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 49 unchanged lines in 6 files lost coverage.
  • Overall coverage increased (+0.04%) to 63.884%

Files with Coverage Reduction New Missed Lines %
common/task/weighted_round_robin_task_scheduler.go 2 88.56%
service/matching/matcher.go 2 90.72%
service/history/task/fetcher.go 4 85.05%
service/history/task/transfer_standby_task_executor.go 4 87.42%
service/history/execution/mutable_state_task_refresher.go 17 61.08%
service/history/task/task_util.go 20 70.57%
Totals Coverage Status
Change from base Build 018e16a9-dd99-45e4-9488-bbd9bb7bd6bd: 0.04%
Covered Lines: 93654
Relevant Lines: 146599

💛 - Coveralls

service/history/shard/context.go Outdated Show resolved Hide resolved
@@ -990,3 +994,274 @@ func (s *mutableStateSuite) buildWorkflowMutableState() *persistence.WorkflowMut
VersionHistories: versionHistories,
}
}

func TestNewMutableStateBuilderWithEventV2(t *testing.T) {

Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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)

},

Copy link
Contributor

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) {

Copy link
Contributor

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)
Copy link
Contributor

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)

Copy link
Contributor Author

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

@davidporter-id-au davidporter-id-au enabled auto-merge (squash) March 7, 2024 00:35
@davidporter-id-au davidporter-id-au merged commit 83f9aca into uber:master Mar 7, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants