-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
Run container orchestrator acceptance tests #13699
Run container orchestrator acceptance tests #13699
Conversation
baf040e
to
fe53803
Compare
@davinchia I found some time to test this out, fixed the tests here so they should be passing now |
unit = TimeUnit.MINUTES) | ||
@EnabledIfEnvironmentVariable(named = "CONTAINER_ORCHESTRATOR", | ||
matches = "true") | ||
public void testCuttingOffPodBeforeFilesTransfer() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this test entirely because it was almost identical to testDowntimeDuringSync and I didn't see the need for it
switch (input) { | ||
case "KILL_BOTH_NON_SYNC_SLIGHTLY_FIRST" -> { | ||
LOGGER.info("Scaling down both workers at roughly the same time..."); | ||
kubernetesClient.apps().deployments().inNamespace("default").withName("airbyte-worker").scale(0); | ||
kubernetesClient.apps().deployments().inNamespace("default").withName("airbyte-sync-worker").scale(0, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these inputs and switch statement, because we don't have any deployments called airbyte-sync-worker
. I think this was leftover from some other changes or something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lmossman ! Appreciate you following up here.
What do you think about moving the ContainerOrchestratorTests into their own specific test class?
The AdvancedAcceptanceTests
are for tests that should run on both Docker and Kubernetes, but the ContainerOrchestrator tests only run when the container orchestrator env is set to true.
I think it would be clearer if this was its own separate test class. The tests in this class would run when KUBE=true
to make clear this is only supported on Kube now. Gradle parallelises at the test class level, so this should decrease the kube acceptance test time by a few minutes. We can decide if we want to add this back, or keep this separate, when we get the container orchestrator working on docker.
0a845c1
to
2309de2
Compare
@davinchia I liked your suggestion, so I went ahead and moved these to their own |
I just realized what you meant about using the |
What
The container-orchestrator acceptance tests are not currently running in CI, because the CONTAINER_ORCHESTRATOR flag is not being set to true in the acceptance test scripts.
How
Set this flag to true in the kube acceptance test scripts, so that container orchestrators are used and the container-orchestrator-specific tests are ran as part of CI.
This makes sense to set to true in the acceptance tests, because that flag is currently set to true by default on master for all kube deployments.
This PR also fixes the tests, as they had some issues when I ran them for the first time.