Bulk Load CDK: Add integration test using in-memory mock destination #45634

edgao · 2024-09-17T22:24:01Z

Add "integration tests" for the CDK, which test full syncs/etc. against an in-memory destination. Very rudimentary for now. Add a new integrationTest task+classpath, to avoid accidentally breaking any micronaut stuff in unit tests. Also updates our github actions (I think?) to call that task as needed.

also - OutputRecord.airbyteMeta is now a proper struct instead of JsonNode; updated RecordDifferTest appropriately. (IntNode / LongNode equality is annoying, in that IntNode(42) != LongNode(42)).

closes https://github.com/airbytehq/airbyte-internal-issues/issues/9960

vercel · 2024-09-17T22:24:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 1, 2024 9:58pm

edgao · 2024-09-17T22:24:16Z

Bulk Load CDK: Add integration test using in-memory mock destination #45634 👈
Load CDK E2ETest Destination w/o tests #45147 : 1 other dependent PR (#44874 )
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @edgao and the rest of your teammates on Graphite

edgao · 2024-09-18T21:19:44Z

airbyte-cdk/bulk/core/load/src/test/kotlin/io/airbyte/cdk/command/MockCatalogFactory.kt

-@Requires(env = ["test"])
+//@Factory
+//@Replaces(factory = DestinationCatalogFactory::class)
+//@Requires(env = ["test"])


@johnny-schmidt I did this yesterday as a quick hack just to get the test to run (otherwise micronaut threw a duplicate bean error). I think the easiest solution is to add another env to the Requires clause? @Requires(env=["test", "mock_catalog_factory"]) and then add that new env to all the tests that rely on this bean?

I could also add some Requires thing to the MockDestinationWrite, but then I'd need to plumb it down to the CliRunner

(... I could also move this stuff to testIntegration and wire up a proper integration test task 🤷 no strong opinion. But I do think it would be nice for the MockCatalogFactory to be an explicit opt-in)

edgao · 2024-09-18T21:20:04Z

airbyte-cdk/bulk/core/load/src/test/resources/metadata.yaml

@@ -0,0 +1,4 @@
+---
+data:
+  dockerRepository: "airbyte/fake-source"


I'll update this file, it's needed to make something in the connector runner happy

.../load/src/testFixtures/kotlin/io/airbyte/cdk/test/write/BasicFunctionalityIntegrationTest.kt

johnny-schmidt · 2024-09-27T00:36:50Z

airbyte-cdk/bulk/core/load/build.gradle

+
+sourceSets {
+    integrationTest {
+    }


Do these require paths?

I guess it defaults?

Is the point to support this: Add a new integrationTest task+classpath, to avoid accidentally breaking any micronaut stuff in unit tests. Also updates our github actions (I think?) to call that task as needed.

Like the point is to prevent connector integration tests from pulling this in? Maybe a clearer/pattern-following name like test-integration-cdk?

welp, my comment on this got hidden #45634 (comment) (tl;dr yes, it populates the paths by default. I should probably just make that an in-code comment 🤔 )

it's more that I'm declaring Singletons without @Requires (b/c I didn't want to add more plumbing into CliRunner/AirbyteConnectorRunner). So putting this stuff into a separate task+classpath (instead of just test) hopefully avoids annoying duplicate bean errors in the cdk unit tests. (connector integration tests wouldn't be affected either way, since they only depend on testFixtures)

so I think integrationTest is the right name? (but also, I don't really feel strongly :P )

test -> cdk unit tests

testFixtures -> cdk tooling for connector tests

integrationTest -> cdk integration tests

.github/workflows/gradle.yml

...ad/src/integrationTest/kotlin/io/airbyte/cdk/mock_integration_test/MockDestinationBackend.kt

.../integrationTest/kotlin/io/airbyte/cdk/mock_integration_test/MockDestinationConfiguration.kt

...oad/src/integrationTest/kotlin/io/airbyte/cdk/mock_integration_test/MockDestinationWriter.kt

.../integrationTest/kotlin/io/airbyte/cdk/mock_integration_test/MockDestinationConfiguration.kt

johnny-schmidt

The mock destination looks great, I like how the test world is shaping up.

I'm not sure about the gradle stuff. Makes sense to run it with the bulk publish but maybe not with the check, in case I'm missing something. All the other ITs explicitly run after check.

Also definitely get eyes on it from sources.

edgao · 2024-10-03T19:31:02Z

talked with @rodireich offline a few days ago and we feel ok with this - @johnny-schmidt can you take another look + approve? (the diff since your last review is just https://github.com/airbytehq/airbyte/pull/45634/files/a718581aaf73fd9d8d5730cdd11988c19a1af413..1454562e7efec83333849e43c539de815e3f7827, i.e. incorporating your idea about ConcurrentHashMap, having check depend on the new task, and getting the build back to green)

johnny-schmidt

LGTM!

postamar

Sorry for the late review. I only looked at the gradle stuff. I'm just confused by the task dependencies because the intent behind them is not clear to me, other than that everything seems fine.

edit: the mustRunAfter check really tripped me up, but it's fine

postamar · 2024-10-07T15:44:30Z

airbyte-cdk/bulk/build.gradle

@@ -61,6 +61,12 @@ allprojects {
    }
 }

+tasks.register('bulkCdkIntegrationTest').configure {
+    // findByName returns the task, or null if no such task exists.
+    // we need this because not all submodules have an integrationTest task.


FYI, alternatively, tasks.matching is also useful for this purpose, perhaps even preferred

postamar · 2024-10-07T15:45:10Z

.github/workflows/publish-bulk-cdk.yml

+          CI: true
+        with:
+          job-id: bulk-cdk-publish
+          concurrent: true


there are some read-only flags that you can set here, not sure if they're actually helpful

in any case none of this seems wrong

postamar · 2024-10-07T15:47:27Z

airbyte-cdk/bulk/core/load/build.gradle

+    testClassesDirs = sourceSets.integrationTest.output.classesDirs
+    classpath = sourceSets.integrationTest.runtimeClasspath
+    useJUnitPlatform()
+    mustRunAfter tasks.check


what's this for? why not have this run as part of check? is this because of the dependency on assemble for the docker image? if it's the latter, it's better to declare that dependency explicitly with dependsOn assemble. These mustRunAfter constraints are typically not what you really want for these kinds of tasks.

this really tripped me up; did you in fact mean to have check depend on integrationTest? if so please let me know @edgao

iirc I copypasted this from stackoverflow without reading it :P I did in fact want to have check depend on integrationTest

(which I think is what I did later on, with the rootProject.check.dependsOn thing? This mustRunAfter thing probably isn't really needed - probably the assumption on SO was that integrationTest is slow, and therefore only worth running if check succeeded. Which isn't true here.)

Thanks!

FYI chatgpt is great at generating gradle scripts

airbyte-cdk/bulk/core/load/build.gradle

This was referenced Sep 17, 2024

Destinations cdk: implement snapshotting spec test #45468

Merged

Destinations CDK: better integration tests #45113

Merged

airbyte-ci: Ignore autogen files #45632

Merged

octavia-squidington-iii added the CDK Connector Development Kit label Sep 17, 2024

This was referenced Sep 17, 2024

Destinations cdk: recorddiffer that handles strongly-typed records #44874

Merged

Load CDK E2ETest Destination w/o tests #45147

Merged

This was referenced Sep 17, 2024

Bulk CDK (Destinations): Use Descriptor instead of full Stream as map key #45610

Merged

Bulk CDK: Add conveniencen method to BufferingOutputConsumer to track message consumption #45611

Merged

edgao commented Sep 18, 2024

View reviewed changes

edgao force-pushed the edgao/spec_test branch from 66dc128 to 9708552 Compare September 19, 2024 21:49

edgao force-pushed the edgao/in_memory_test branch from 2e326f0 to f377e2a Compare September 19, 2024 21:49

edgao force-pushed the edgao/spec_test branch from 9708552 to 23585e5 Compare September 19, 2024 21:54

edgao force-pushed the edgao/in_memory_test branch from f377e2a to 5fd6001 Compare September 19, 2024 21:54

edgao force-pushed the edgao/spec_test branch from 23585e5 to ea8314f Compare September 19, 2024 21:56

edgao force-pushed the edgao/in_memory_test branch from 5fd6001 to 371f6c7 Compare September 19, 2024 21:56

edgao force-pushed the edgao/spec_test branch from ea8314f to 7c7d04d Compare September 19, 2024 21:59

edgao force-pushed the edgao/in_memory_test branch from 371f6c7 to f576bce Compare September 19, 2024 22:00

edgao force-pushed the edgao/spec_test branch from 7c7d04d to d4b2c4e Compare September 20, 2024 15:24

edgao force-pushed the edgao/in_memory_test branch from f576bce to 40e7de2 Compare September 20, 2024 15:24

edgao force-pushed the edgao/spec_test branch from d4b2c4e to f20b33d Compare September 20, 2024 16:42

edgao force-pushed the edgao/in_memory_test branch from 40e7de2 to 3b39969 Compare September 20, 2024 16:42

edgao force-pushed the edgao/spec_test branch from f20b33d to 9511fda Compare September 20, 2024 18:20

edgao force-pushed the edgao/in_memory_test branch 2 times, most recently from 5da57c2 to 9d49f7d Compare September 20, 2024 18:38

Base automatically changed from edgao/spec_test to issue-9361/load-cdk-with-e2e-dest-post-refactor September 20, 2024 20:02

edgao force-pushed the issue-9361/load-cdk-with-e2e-dest-post-refactor branch from 9511fda to 391756a Compare September 20, 2024 21:04

edgao force-pushed the edgao/in_memory_test branch from 9d49f7d to 7c5df8d Compare September 20, 2024 21:04

johnny-schmidt force-pushed the issue-9361/load-cdk-with-e2e-dest-post-refactor branch 2 times, most recently from cfb0cc1 to 91e6e18 Compare September 26, 2024 17:56

edgao force-pushed the edgao/in_memory_test branch from e3a99c2 to 9e8312a Compare September 26, 2024 18:38

johnny-schmidt force-pushed the issue-9361/load-cdk-with-e2e-dest-post-refactor branch 3 times, most recently from 3c6895c to 096dc33 Compare September 26, 2024 20:57

Base automatically changed from issue-9361/load-cdk-with-e2e-dest-post-refactor to master September 26, 2024 21:24

edgao force-pushed the edgao/in_memory_test branch from 9e8312a to 9797a8a Compare September 26, 2024 22:01

edgao commented Sep 26, 2024

View reviewed changes

.../load/src/testFixtures/kotlin/io/airbyte/cdk/test/write/BasicFunctionalityIntegrationTest.kt Show resolved Hide resolved