add unit test for MigrationInterceptor #956
Conversation
shijiesheng
requested review from
Groxx,
abhishekj720,
natemort,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
| .withDomain(DOMAIN) | ||
| .withWorkflowTypes(CrossDomainWorkflowTest.TestWorkflowCrossDomainImpl.class) | ||
| .startWorkersAutomatically() | ||
| .withTestEnvironmentProvider(TestWorkflowEnvironment::newInstance) |
There was a problem hiding this comment.
This is the default value of TestEnvironmentProvider, you can remove. The CrossDomainWorkflowTest had to specify it to share one environment across both workers since it had workflows signaling across domains.
| public CadenceTestRule testRuleCur = | ||
| CadenceTestRule.builder() | ||
| .withDomain(DOMAIN) | ||
| .withWorkflowTypes(CrossDomainWorkflowTest.TestWorkflowCrossDomainImpl.class) |
There was a problem hiding this comment.
Is this workflow used within the test? I think you can remove both withWorkflowTypes and startWorkersAutomatically since the test seems to create its own workers.
There was a problem hiding this comment.
make sense. I missed this
| new FeatureFlags().setWorkflowExecutionAlreadyCompletedErrorEnabled(true)) | ||
| .build()); | ||
| IWorkflowService serviceNew, serviceCur; | ||
| if (useDockerService) { |
There was a problem hiding this comment.
Instead of the conditional logic you should be able to use the else branch for both cases. If useDockerService is set to true then the IWorkflowClient in the test rules will be thrift clients.
| import org.junit.Assume; | ||
| import org.junit.Before; | ||
| import org.junit.Test; | ||
| import org.junit.*; | ||
|
|
||
| public class WorkflowMigrationTest { |
There was a problem hiding this comment.
Nit: See if you can remove useDockerService entirely, the test rule should handle most cases.
There was a problem hiding this comment.
I do want to have an integration test from end to end.
There was a problem hiding this comment.
Sorry, I don't think I explained this well, but the CadenceTestRule has the conditional logic within it already: https://github.com/uber/cadence-java-client/blob/704380bfaed0aa873fa19fe01d7bb0645c690c53/src/test/java/com/uber/cadence/testUtils/CadenceTestRule.java#L196
If USE_DOCKER_SERVICE is set to true then CadenceTestRule will connect to the docker instance instead of creating a test service. The goal of the test rule is to avoid every test needing to have conditional logic.
There was a problem hiding this comment.
Oh, I don't know that. I'll remove it completely
| @@ -155,7 +179,7 @@ public void execute(int iter) { | |||
|
|
|||
| @Test | |||
| public void whenUseDockerService_cronWorkflowMigration() { | |||
| Assume.assumeTrue(useDockerService); | |||
| // Assume.assumeTrue(useDockerService); | |||
| } catch (Exception eDesc) { | ||
| fail("fail to describe workflow execution in new domain: " + eDesc); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private DescribeWorkflowExecutionResponse describeWorkflowExecution( | ||
| private GetWorkflowExecutionHistoryResponse getWorkflowHistory( |
There was a problem hiding this comment.
Nit: Why change the type? Does this cover a different code path?
There was a problem hiding this comment.
describe method is not implemented in test service
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted filessee 23 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
What changed?
Why?
unit test coverage
How did you test it?
Unit test
Potential risks
Release notes
Documentation Changes