Add acceptance test for deleting connetion #11563

terencecho · 2022-03-30T04:20:40Z

What

Add an acceptance test to check that we are able to delete a connection during a normal state and even when the state of the temporal workflow is in a terminal state. This behavior was originally committed in this PR: #11302 with unit tests and verified manually, but lacked an acceptance test.

terencecho · 2022-03-30T17:16:23Z

running into this error in the github builds

AcceptanceTests > testActionsWhenTemporalIsInTerminalState() FAILED
    io.grpc.StatusRuntimeException: NOT_FOUND: sql: no rows in result set
        at app//io.grpc.stub.ClientCalls.toStatusRuntimeException(ClientCalls.java:262)
        at app//io.grpc.stub.ClientCalls.getUnchecked(ClientCalls.java:243)
        at app//io.grpc.stub.ClientCalls.blockingUnaryCall(ClientCalls.java:156)
        at app//io.temporal.api.workflowservice.v1.WorkflowServiceGrpc$WorkflowServiceBlockingStub.terminateWorkflowExecution(WorkflowServiceGrpc.java:2884)
        at app//io.temporal.internal.client.external.GenericWorkflowClientExternalImpl.lambda$terminate$4(GenericWorkflowClientExternalImpl.java:202)
        at app//io.temporal.internal.retryer.GrpcRetryer.lambda$retry$0(GrpcRetryer.java:44)
        at app//io.temporal.internal.retryer.GrpcSyncRetryer.retry(GrpcSyncRetryer.java:61)
        at app//io.temporal.internal.retryer.GrpcRetryer.retryWithResult(GrpcRetryer.java:51)
        at app//io.temporal.internal.retryer.GrpcRetryer.retry(GrpcRetryer.java:41)
        at app//io.temporal.internal.client.external.GenericWorkflowClientExternalImpl.terminate(GenericWorkflowClientExternalImpl.java:195)
        at app//io.temporal.internal.client.RootWorkflowClientInvoker.terminate(RootWorkflowClientInvoker.java:174)
        at app//io.temporal.internal.sync.WorkflowStubImpl.terminate(WorkflowStubImpl.java:376)
        at app//io.airbyte.test.acceptance.AcceptanceTests.testActionsWhenTemporalIsInTerminalState(AcceptanceTests.java:1187)

which seems to suggest that it's having problems finding the workflow in the temporal database.

This reverts commit 0e53a27.

terencecho · 2022-03-30T23:47:27Z

I found it weird that it didn’t work when i tried

@EnabledIfEnvironmentVariable(named = "NEW_SCHEDULER",
                              matches = "true")

but worked when i did the following

    final FeatureFlags featureFlags = new EnvVariableFeatureFlags();
    if (!featureFlags.usesNewScheduler()) {
      LOGGER.info("Skipping test since not using new temporal scheduler");
      return;
    }

although i think if i can get the first approach to work, it would be better, since the log lines were not outputted here. I would need to find where to access the standard output of the acceptance tests. I verified that the test was actually ran by running it locally.

benmoriceau

Thanks for improving the acceptance coverage! I only have minor comments.

benmoriceau · 2022-03-31T00:00:50Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/AcceptanceTests.java

+
+    // Terminate workflow
+    LOGGER.info("Terminating temporal workflow...");
+    workflowCLient.newUntypedWorkflowStub("connection_manager_" + connectionId).terminate("");


Can we extract the termination in its own method? It will be needed for other tests.

benmoriceau · 2022-03-31T00:01:47Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/AcceptanceTests.java

+    workflowCLient.newUntypedWorkflowStub("connection_manager_" + connectionId).terminate("");
+
+    // expect an exception because the temporal workflow is not running
+    Assertions.assertThatExceptionOfType(ApiException.class)


We should remove that, it won't throw exception later on.

makes sense. removed

benmoriceau · 2022-03-31T00:03:04Z

airbyte-tests/src/acceptanceTests/java/io/airbyte/test/acceptance/AcceptanceTests.java

@@ -1147,6 +1152,57 @@ public void testCancelSyncWhenCancelledWhenWorkerIsNotRunning() throws Exception
    assertEquals(JobStatus.CANCELLED, resp.get().getJob().getStatus());
  }

+  @Test
+  @Order(22)
+  @DisabledIfEnvironmentVariable(named = "KUBE",


Is there a reason to not run on Kube?

originally i thought we would have to do some extra stuff to get it working on kube, but i guess that's not the case. it's been removed.

terencecho · 2022-03-31T04:17:35Z

@benmoriceau PTAL. i refactored it to cover both the normal case and the temporal in a bad state case.

Add acceptance test for deleting connetion in bad temporal state

dd111ed

terencecho temporarily deployed to more-secrets March 30, 2022 04:21 Inactive

terencecho temporarily deployed to more-secrets March 30, 2022 05:01 Inactive

terencecho temporarily deployed to more-secrets March 30, 2022 05:04 Inactive

disable new test on kube

04f7f9f

terencecho temporarily deployed to more-secrets March 30, 2022 06:24 Inactive

try using different temproalHost

dbcd832

terencecho temporarily deployed to more-secrets March 30, 2022 07:29 Inactive

terencecho temporarily deployed to more-secrets March 30, 2022 07:30 Inactive

add log info line to give time for temporal client to spin up

0922758

terencecho temporarily deployed to more-secrets March 30, 2022 08:00 Inactive

try avoiding missing row in temporal db

fecc56e

check if temporal workflow is reachable

7b30eee

terencecho temporarily deployed to more-secrets March 30, 2022 18:37 Inactive

terencecho temporarily deployed to more-secrets March 30, 2022 18:38 Inactive

terencecho added 3 commits March 30, 2022 11:51

check if temporal workflow is reachable

770a885

check if temporal workflow is reachable

2e90a21

try waiting for connection state

6cb3181

terencecho temporarily deployed to more-secrets March 30, 2022 19:46 Inactive

terencecho temporarily deployed to more-secrets March 30, 2022 20:09 Inactive

try using airbyte-temporal hostname

0e53a27

terencecho temporarily deployed to more-secrets March 30, 2022 20:17 Inactive

terencecho added 2 commits March 30, 2022 13:54

Revert "try using airbyte-temporal hostname"

95438ef

This reverts commit 0e53a27.

Revert back to using localhost

2828cc9

Add 5 second wait

e33be17

terencecho temporarily deployed to more-secrets March 30, 2022 21:06 Inactive

only enable test for new scheduler

6fb466e

terencecho temporarily deployed to more-secrets March 30, 2022 22:23 Inactive

only enable test for new scheduler 2

7cb16f6

terencecho temporarily deployed to more-secrets March 30, 2022 23:19 Inactive

terencecho requested a review from benmoriceau March 30, 2022 23:46

benmoriceau reviewed Mar 31, 2022

View reviewed changes

refactor test to cover normal and unexpected temporal state

1b3d7bb

terencecho changed the title ~~Add acceptance test for deleting connetion in bad temporal state~~ Add acceptance test for deleting connetion Mar 31, 2022

terencecho temporarily deployed to more-secrets March 31, 2022 03:48 Inactive

terencecho requested a review from benmoriceau March 31, 2022 04:17

terencecho mentioned this pull request Mar 31, 2022

[Unexpected state] Delete connection #11214

Closed

benmoriceau approved these changes Apr 4, 2022

View reviewed changes

terencecho merged commit 568f112 into master Apr 4, 2022

terencecho deleted the tcho/11214-acceptance-tests branch April 4, 2022 17:51

octavia-squidington-iii mentioned this pull request Apr 5, 2022

Bump Airbyte version from 0.35.64-alpha to 0.35.65-alpha #11722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add acceptance test for deleting connetion #11563

Add acceptance test for deleting connetion #11563

terencecho commented Mar 30, 2022 •

edited

Loading

terencecho commented Mar 30, 2022

terencecho commented Mar 30, 2022 •

edited

Loading

benmoriceau left a comment

benmoriceau Mar 31, 2022

terencecho Mar 31, 2022

benmoriceau Mar 31, 2022

terencecho Mar 31, 2022

benmoriceau Mar 31, 2022

terencecho Mar 31, 2022

terencecho commented Mar 31, 2022

Add acceptance test for deleting connetion #11563

Add acceptance test for deleting connetion #11563

Conversation

terencecho commented Mar 30, 2022 • edited Loading

What

terencecho commented Mar 30, 2022

terencecho commented Mar 30, 2022 • edited Loading

benmoriceau left a comment

Choose a reason for hiding this comment

benmoriceau Mar 31, 2022

Choose a reason for hiding this comment

terencecho Mar 31, 2022

Choose a reason for hiding this comment

benmoriceau Mar 31, 2022

Choose a reason for hiding this comment

terencecho Mar 31, 2022

Choose a reason for hiding this comment

benmoriceau Mar 31, 2022

Choose a reason for hiding this comment

terencecho Mar 31, 2022

Choose a reason for hiding this comment

terencecho commented Mar 31, 2022

terencecho commented Mar 30, 2022 •

edited

Loading

terencecho commented Mar 30, 2022 •

edited

Loading