Skip to content
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

add unit test for MigrationInterceptor #956

Merged
merged 3 commits into from
Nov 8, 2024
Merged

add unit test for MigrationInterceptor #956

Show file tree
Hide file tree
Changes from all commits
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());
}
}
45 changes: 19 additions & 26 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 @@ -30,9 +30,8 @@
import com.uber.cadence.migration.MigrationActivitiesImpl;
import com.uber.cadence.migration.MigrationIWorkflowService;
import com.uber.cadence.migration.MigrationInterceptorFactory;
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.worker.Worker;
import com.uber.cadence.worker.WorkerFactory;
import com.uber.cadence.worker.WorkerFactoryOptions;
Expand All @@ -41,37 +40,33 @@
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;
private boolean useDockerService = Boolean.parseBoolean(System.getenv("USE_DOCKER_SERVICE"));
private static final String TASKLIST = "TASKLIST";
private TracingWorkflowInterceptorFactory tracer;
WorkerFactory factoryCurr, factoryNew;
Worker workerCurr, workerNew;

@Rule public CadenceTestRule testRuleCur = CadenceTestRule.builder().withDomain(DOMAIN).build();

@Rule public CadenceTestRule testRuleNew = CadenceTestRule.builder().withDomain(DOMAIN2).build();

@Before
public void setUp() {
IWorkflowService service =
new WorkflowServiceTChannel(
ClientOptions.newBuilder()
.setFeatureFlags(
new FeatureFlags().setWorkflowExecutionAlreadyCompletedErrorEnabled(true))
.build());
IWorkflowService serviceCur = testRuleCur.getWorkflowClient().getService();
IWorkflowService 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 @@ -154,8 +149,7 @@ public void execute(int iter) {
}

@Test
public void whenUseDockerService_cronWorkflowMigration() {
Assume.assumeTrue(useDockerService);
public void cronWorkflowMigration() {
String workflowID = UUID.randomUUID().toString();
try {
workflowClientCurr
Expand All @@ -164,16 +158,15 @@ 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);
}
}
}

@Test
public void whenUseDockerService_continueAsNewWorkflowMigration() {
Assume.assumeTrue(useDockerService);
public void continueAsNewWorkflowMigration() {
String workflowID = UUID.randomUUID().toString();
try {
workflowClientCurr
Expand All @@ -183,18 +176,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