[code-coverage] Add tests for mutable_state_util.go #6062
Conversation
ketsiambaku
changed the title
ketsiambaku
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
ketsiambaku
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 40 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Pull Request Test Coverage Report for Build 018fdb1f-40a9-4f14-ab60-448573a7c3cfDetails
💛 - Coveralls |
| expected := testdata.DomainName | ||
| mockDomainCache.EXPECT().GetDomainName(childInfo.DomainID).Return(expected, nil) | ||
| name, err := GetChildExecutionDomainName(childInfo, mockDomainCache, constants.TestLocalDomainEntry) | ||
| assert.Nil(t, err) |
There was a problem hiding this comment.
assert.NoError() is a better candidate for error-checking.
Also, I recommend using require in this case since most probably you're not interested in checking for value if err != nil; in fact, in lots of cases it will even panic.
| builder.decisionTaskManager.(*mutableStateDecisionTaskManagerImpl).HasInFlightDecision() | ||
| builder.executionInfo.DecisionStartedID = 1 | ||
|
|
||
| mutableState := CreatePersistenceMutableState(builder) |
There was a problem hiding this comment.
This is a mapper, taking a type (mutableStateBuilder) and creating a persistence Mutable State. Is it possible to see if there's the reverse somewhere?
This kind of code is quite prone to not getting updated, I feel fairly strongly that we should round-trip test these mappings rather than just unit-testing them. Happy to sync to discuss what I mean.
See TestMutableStateBuilder_CopyToPersistence_roundtrip for an example of a roundtrip test.
There was a problem hiding this comment.
the msb.CopyToPersistance() method I was testing looks weirdly similar, but I think they're different. Not super clear, but I think this one is a deep copy, so modifications to the copied persistence struct shouldn't affect the original maybe? not super sure how rigorous that is.
There was a problem hiding this comment.
Sure, I can add the roundtrip test on a follow up PR. I'm looking into the example.
ketsiambaku
force-pushed
the
mutable-state-util
branch
from
d96623b to
bb0c768
Compare
…#6062) * Add tests for mutable_state_util_test.go * added mutablestate assertions
…#6062) * Add tests for mutable_state_util_test.go * added mutablestate assertions
What changed?
Added tests to improve code coverage in service/history/execution/mutable_state_util_test.go
Why?
Code Coverage Initiative 📈
How did you test it?
Potential risks
Release notes
Documentation Changes