Add test for history_replicator, ApplyEvent function #6004
Conversation
bowenxia
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| eventsAffordance func() []*types.HistoryEvent | ||
| expectedErrorMsg string | ||
| }{ | ||
| "Case1: empty case": { |
There was a problem hiding this comment.
remark, no action required: I generally suggest starting from the full case first since it's the most laborious, and then working through the exception cases, it is just easier to delete stuff than to add it
There was a problem hiding this comment.
actually, are all of these jsut error cases? Is there a test somewhere where we verify a successful application?
There was a problem hiding this comment.
Not yet. There are so many branches in one function, I was trying to build a full case first, but looks like it is more efficient to build the successful case step by step from different fail cases gradually.
| @@ -357,3 +360,196 @@ func TestNewHistoryReplicator_newMutableState(t *testing.T) { | |||
| ) | |||
| assert.NotNil(t, testReplicatorImpl.newMutableState(mockDomainCacheEntry, log.NewNoop())) | |||
| } | |||
|
|
|||
| func TestApplyEvents_newReplicationTask_validateReplicateEventsRequest(t *testing.T) { | |||
There was a problem hiding this comment.
It looks like all the errors are from validateReplicateEventsRequest.
Might consider creating some function parameters and overwriting them in unit tests to mock these functions.
And you can write tests for individual functions separately to test those functions referenced by ApplyEvents.
Here is an example you can follow: #5971
There was a problem hiding this comment.
Amazing idea! I've made refactors according to your advice. Please take another look when you got some time. 🙏
Pull Request Test Coverage Report for Build 018f7345-3b84-4b56-91fc-2d821c405a8fDetails
💛 - Coveralls |
|
@Shaddoll Yes. I'll add success case later in another PR since it will need more refactor in applyEvent function. |
…w#6004) * Add test for history_replicator, ApplyEvent function * refactor code to pass in functions for testing * reformat * delete a duplicate test case
What changed?
Add test for ApplyEvent function in history_replicator.go, and NewReplicationTask in replication_task.go.
Refactored code to have newReplicationTask function as a parameter of historyReplicatorImpl.
Why?
Code coverage week.
Use newReplicationTaskFn as a parameter is to increase the testability. Doing it this way, we can pass in mock newReplicationTaskFn functions so that we can test ApplyEvent function as a whole, and test the functions it used in different unit tests.
How did you test it?
unit test
Potential risks
Release notes
Documentation Changes