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

added units test for functions in mapper/thrift/replicator.go #5835

Conversation

d-vignesh
Copy link
Contributor

@d-vignesh d-vignesh commented Apr 2, 2024

What changed?
Added unit test for function in common/types/mapper/thrift/replicator.go

Why?
Improve code coverage

How did you test it?
local testing with go test.

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

Merging #5835 (531cf2b) into master (b28dca3) will increase coverage by 0.24%.
The diff coverage is n/a.

❗ Current head 531cf2b differs from pull request most recent head 6ae6836. Consider uploading reports for the commit 6ae6836 to get more accurate results

Additional details and impacted files
Files Coverage Δ
common/types/mapper/thrift/replicator.go 96.56% <ø> (+93.89%) ⬆️

... and 8 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 b28dca3...6ae6836. Read the comment docs.

@coveralls
Copy link

coveralls commented Apr 3, 2024

Pull Request Test Coverage Report for Build 018eb06e-c4cf-4253-8e83-ab9e5926fa70

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 41 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+0.4%) to 67.179%

Files with Coverage Reduction New Missed Lines %
common/persistence/sql/sqlplugin/mysql/task.go 2 73.68%
service/matching/db.go 2 73.23%
service/history/execution/mutable_state_builder.go 2 70.28%
service/matching/matcher.go 2 90.72%
common/persistence/sql/sqlplugin/mysql/db.go 2 79.49%
common/log/tag/tags.go 3 50.74%
common/task/fifo_task_scheduler.go 3 84.54%
common/persistence/statsComputer.go 3 96.07%
service/history/task/fetcher.go 3 85.57%
service/history/task/transfer_standby_task_executor.go 4 87.42%
Totals Coverage Status
Change from base Build 018eb05e-a5e5-4ec0-b4c3-a6b9b0eebad2: 0.4%
Covered Lines: 98067
Relevant Lines: 145979

💛 - Coveralls

@taylanisikdemir taylanisikdemir changed the title added units test for functions in common/types/mapper/thrift/replicat… added units test for functions in mapper/thrift/replicator.go Apr 3, 2024
@@ -141,4 +147,40 @@ var (
FailoverVersion: FailoverVersion1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. I guess this file existing is a decent argument that we do need a random-data factory :\

oh well. additions look reasonable 👍

Copy link
Contributor

@Groxx Groxx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some very minor "might be nicer if..." thoughts while reading, but overall 👍 nice and simple, big jump in coverage, thanks!

if you want to make any changes I can hold off on merging, but this looks more than good enough to me as-is.
or I'll just merge for you tomorrow if I don't hear back :)

Comment on lines +218 to +236
testCases := []struct {
desc string
input *types.GetDomainReplicationMessagesRequest
}{
{
desc: "non-nil input test",
input: &testdata.GetDomainReplicationMessagesRequest,
},
{
desc: "empty input test",
input: &types.GetDomainReplicationMessagesRequest{},
},
{
desc: "nil input test",
input: nil,
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
Copy link
Contributor

@Groxx Groxx Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a thing we've been kinda trying to shift towards / for minor test-boilerplate improvement, making the test container a map[string] can be convenient:

Suggested change
testCases := []struct {
desc string
input *types.GetDomainReplicationMessagesRequest
}{
{
desc: "non-nil input test",
input: &testdata.GetDomainReplicationMessagesRequest,
},
{
desc: "empty input test",
input: &types.GetDomainReplicationMessagesRequest{},
},
{
desc: "nil input test",
input: nil,
},
}
for _, tc := range testCases {
t.Run(tc.desc, func(t *testing.T) {
testCases := map[string]*types.GetDomainReplicationMessagesRequest{
"non-nil": &testdata.GetDomainReplicationMessagesRequest,
"empty": &types.GetDomainReplicationMessagesRequest{},
"nil": nil,
}
for name, tc := range testCases {
t.Run(name, func(t *testing.T) {

we obviously mostly don't do this though, your []struct is overwhelmingly the pattern in these tests, so this is not at all a request to change anything. just an observation for anyone reading / reviewing / etc for future work :)

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 @Groxx will follow the map pattern for future tests.

const TaskType = int16(1)
const (
TaskType = int16(1)
MaxiumuPageSize = int32(5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
MaxiumuPageSize = int32(5)
MaximumPageSize = int32(5)

@d-vignesh
Copy link
Contributor Author

Hi @Groxx, thank you for the review, if things look fine from your end, we can go ahead and merge it.

@Groxx
Copy link
Contributor

Groxx commented Apr 4, 2024

Hi @Groxx, thank you for the review, if things look fine from your end, we can go ahead and merge it.

ah, maybe you don't have a merge button after approval.
oh well. I'll do that now :)

@agautam478 agautam478 enabled auto-merge (squash) April 4, 2024 20:08
@Groxx Groxx disabled auto-merge April 5, 2024 20:53
@Groxx Groxx enabled auto-merge (squash) April 5, 2024 20:54
@Groxx Groxx merged commit 3479700 into cadence-workflow:master Apr 5, 2024
17 of 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.

7 participants