Added tests to service/worker/scanner.go #6349
Conversation
What changed? Modified scanner.go to accept tests. Added tests to scanner.go. Why? Improve code coverage. How did you test it? Unit tests Potential risks Release notes Documentation Changes
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
... and 40 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
fimanishi
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
Groxx,
shijiesheng,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| @@ -188,7 +193,7 @@ func (s *Scanner) startShardScanner( | |||
| config.ScannerWFTypeName, | |||
| shardscanner.NewShardScannerContext(s.context.resource, config), | |||
| ) | |||
| go workercommon.StartWorkflowWithRetry( | |||
There was a problem hiding this comment.
func StartWorkflowWithRetry(...) error {
// ...
sdkClient := client.NewClient(😱
well. a lot of this is unavoidable I guess. we can tackle that mess another time.
| }, | ||
| }, | ||
| setupMocks: func() { | ||
| s.mockWorker.EXPECT().Start().Return(nil).Times(2) |
There was a problem hiding this comment.
maybe worth a comment in code here:
it's a test-suite quirk that this is the same mockWorker started 2x. normally there would be two separate workers, each started once.
which seems fine for these tests (I don't really think it'd be worth asserting "makes two and starts both", that seems like it could be a lot of work for code that obviously does that), it's just misleading to look at.
(maybe it'd be worth trying to assert "constructor called 2x"? idk, I really think it's fine as-is, this isn't a very high-value check)
| setupMocks: func(options client.StartWorkflowOptions, workflowType string, args interface{}) { | ||
| s.mockClient.On("StartWorkflow", mock.Anything, options, workflowType).Return(nil, &shared.WorkflowExecutionAlreadyStartedError{}) | ||
| }, | ||
| }, |
There was a problem hiding this comment.
personally: since this is a bit of an exception (return err but expect no err), I like having an explicit
err: nil,to show it was thought about and is nil on purpose.
(for the others above, where it's kinda obvious it shouldn't error: eh, it's one fewer line, I'd leave it out too)
What changed?
Modified scanner.go to accept tests.
Added tests to scanner.go.
There was no mock for worker. Created an interface and auto generated the mocks.
Why?
Improve code coverage.
How did you test it?
Unit tests
Potential risks
Release notes
Documentation Changes