Skip to content

add unit test for MigrationInterceptor #956

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 8, 2024
Merged

add unit test for MigrationInterceptor #956

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71f40e2
add unit test for MigrationInterceptor
shijiesheng Nov 7, 2024
235a768
address comments
shijiesheng Nov 7, 2024
c8f85be
address comments to remove use docker
shijiesheng Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/test/java/com/uber/cadence/migration/MigrationIWorkflowServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static org.mockito.Mockito.*;

import com.uber.cadence.*;
import com.uber.cadence.serviceclient.ClientOptions;
import com.uber.cadence.serviceclient.IWorkflowService;
import java.util.ArrayList;
import org.apache.thrift.TException;
Expand Down Expand Up @@ -1218,4 +1219,12 @@ public void testSignalWorkflowExecution_SignalInOldService() throws TException {
verifyNoMoreInteractions(serviceNew);
verifyNoMoreInteractions(serviceOld);
}

@Test
public void testGetOptions() {
when(serviceOld.getOptions())
.thenReturn(ClientOptions.newBuilder().setServiceName("serviceName").build());
ClientOptions options = migrationService.getOptions();
assertEquals("serviceName", options.getServiceName());
}
}
65 changes: 44 additions & 21 deletions src/test/java/com/uber/cadence/workflow/WorkflowMigrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
import com.uber.cadence.serviceclient.ClientOptions;
import com.uber.cadence.serviceclient.IWorkflowService;
import com.uber.cadence.serviceclient.WorkflowServiceTChannel;
import com.uber.cadence.testUtils.CadenceTestRule;
import com.uber.cadence.testing.TestWorkflowEnvironment;
import com.uber.cadence.worker.Worker;
import com.uber.cadence.worker.WorkerFactory;
import com.uber.cadence.worker.WorkerFactoryOptions;
Expand All @@ -41,10 +43,7 @@
import java.util.UUID;
import java.util.concurrent.CancellationException;
import org.apache.thrift.TException;
import org.junit.After;
import org.junit.Assume;
import org.junit.Before;
import org.junit.Test;
import org.junit.*;

public class WorkflowMigrationTest {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: See if you can remove useDockerService entirely, the test rule should handle most cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do want to have an integration test from end to end.

Copy link
Member

@natemort natemort Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I don't know that. I'll remove it completely

private WorkflowClient migrationWorkflowClient, workflowClientCurr, workflowClientNew;
Expand All @@ -54,24 +53,49 @@ public class WorkflowMigrationTest {
WorkerFactory factoryCurr, factoryNew;
Worker workerCurr, workerNew;

@Rule
public CadenceTestRule testRuleCur =
CadenceTestRule.builder()
.withDomain(DOMAIN)
.withWorkflowTypes(CrossDomainWorkflowTest.TestWorkflowCrossDomainImpl.class)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

make sense. I missed this

.startWorkersAutomatically()
.withTestEnvironmentProvider(TestWorkflowEnvironment::newInstance)
Copy link
Member

Choose a reason for hiding this comment

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

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.

.build();

@Rule
public CadenceTestRule testRuleNew =
CadenceTestRule.builder()
.withDomain(DOMAIN2)
.withWorkflowTypes(WorkflowTest.TestWorkflowSignaledSimple.class)
.startWorkersAutomatically()
.withTestEnvironmentProvider(TestWorkflowEnvironment::newInstance)
.build();

@Before
public void setUp() {
IWorkflowService service =
new WorkflowServiceTChannel(
ClientOptions.newBuilder()
.setFeatureFlags(
new FeatureFlags().setWorkflowExecutionAlreadyCompletedErrorEnabled(true))
.build());
IWorkflowService serviceNew, serviceCur;
if (useDockerService) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

make sense

serviceCur =
new WorkflowServiceTChannel(
ClientOptions.newBuilder()
.setFeatureFlags(
new FeatureFlags().setWorkflowExecutionAlreadyCompletedErrorEnabled(true))
.build());
serviceNew = serviceCur; // docker only starts one server so share the same service
} else {
serviceCur = testRuleCur.getWorkflowClient().getService();
serviceNew = testRuleNew.getWorkflowClient().getService();
}
workflowClientCurr =
WorkflowClient.newInstance(
service, WorkflowClientOptions.newBuilder().setDomain(DOMAIN).build());
serviceCur, WorkflowClientOptions.newBuilder().setDomain(DOMAIN).build());
workflowClientNew =
WorkflowClient.newInstance(
service, WorkflowClientOptions.newBuilder().setDomain(DOMAIN2).build());
serviceNew, WorkflowClientOptions.newBuilder().setDomain(DOMAIN2).build());
MigrationIWorkflowService migrationService =
new MigrationIWorkflowService(
service, DOMAIN,
service, DOMAIN2);
serviceCur, DOMAIN,
serviceNew, DOMAIN2);
migrationWorkflowClient =
WorkflowClient.newInstance(
migrationService, WorkflowClientOptions.newBuilder().setDomain(DOMAIN).build());
Expand Down Expand Up @@ -155,7 +179,7 @@ public void execute(int iter) {

@Test
public void whenUseDockerService_cronWorkflowMigration() {
Assume.assumeTrue(useDockerService);
// Assume.assumeTrue(useDockerService);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: remove comment

String workflowID = UUID.randomUUID().toString();
try {
workflowClientCurr
Expand All @@ -164,7 +188,7 @@ public void whenUseDockerService_cronWorkflowMigration() {
.execute("for test");
} catch (CancellationException e) {
try {
describeWorkflowExecution(workflowClientNew, workflowID);
getWorkflowHistory(workflowClientNew, workflowID);
} catch (Exception eDesc) {
fail("fail to describe workflow execution in new domain: " + eDesc);
}
Expand All @@ -173,7 +197,6 @@ public void whenUseDockerService_cronWorkflowMigration() {

@Test
public void whenUseDockerService_continueAsNewWorkflowMigration() {
Assume.assumeTrue(useDockerService);
String workflowID = UUID.randomUUID().toString();
try {
workflowClientCurr
Expand All @@ -183,18 +206,18 @@ public void whenUseDockerService_continueAsNewWorkflowMigration() {
.execute(0);
} catch (CancellationException e) {
try {
describeWorkflowExecution(workflowClientNew, workflowID);
getWorkflowHistory(workflowClientNew, workflowID);
} catch (Exception eDesc) {
fail("fail to describe workflow execution in new domain: " + eDesc);
}
}
}

private DescribeWorkflowExecutionResponse describeWorkflowExecution(
private GetWorkflowExecutionHistoryResponse getWorkflowHistory(
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Why change the type? Does this cover a different code path?

Copy link
Member Author

Choose a reason for hiding this comment

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

describe method is not implemented in test service

WorkflowClient wc, String workflowID) throws TException {
return wc.getService()
.DescribeWorkflowExecution(
new DescribeWorkflowExecutionRequest()
.GetWorkflowExecutionHistory(
new GetWorkflowExecutionHistoryRequest()
.setExecution(new WorkflowExecution().setWorkflowId(workflowID))
.setDomain(wc.getOptions().getDomain()));
}
Expand Down