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.

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

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

Merged
merged 5 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/gradle.yml
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ jobs:
gradle-distribution-sha-256-sum-warning: false
concurrent: true
# TODO: be able to remove the skipSlowTests property
arguments: --scan check :airbyte-cdk:bulk:bulkCdkIntegrationTest -DskipSlowTests=true
arguments: --scan check -DskipSlowTests=true

set-instatus-incident-on-failure:
name: Create Instatus Incident on Failure
Expand Down
6 changes: 4 additions & 2 deletions airbyte-cdk/bulk/core/load/build.gradle
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// simply declaring the source sets is sufficient to populate them with
// src/integrationTest/java+resources + src/integrationTest/kotlin.
sourceSets {
integrationTest {
}
}

kotlin {
sourceSets {
testIntegration {
Expand Down Expand Up @@ -31,8 +32,9 @@ task integrationTest(type: Test) {
useJUnitPlatform()
mustRunAfter tasks.check
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks!

FYI chatgpt is great at generating gradle scripts

}

configurations {
integrationTestImplementation.extendsFrom testImplementation
integrationTestRuntimeOnly.extendsFrom testRuntimeOnly
}
// These tests are lightweight enough to run on every PR.
rootProject.check.dependsOn(integrationTest)
edgao marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,21 @@ package io.airbyte.cdk.mock_integration_test

import io.airbyte.cdk.test.util.DestinationDataDumper
import io.airbyte.cdk.test.util.OutputRecord
import java.util.concurrent.ConcurrentHashMap

object MockDestinationBackend {
private val lock = Object()
private val files: MutableMap<String, MutableList<OutputRecord>> = mutableMapOf()
private val files: MutableMap<String, MutableList<OutputRecord>> = ConcurrentHashMap()

fun insert(filename: String, vararg records: OutputRecord) {
synchronized(lock) { getFile(filename).addAll(records) }
getFile(filename).addAll(records)
}

fun readFile(filename: String): List<OutputRecord> {
return synchronized(lock) {
getFile(filename)
}
return getFile(filename)
}

private fun getFile(filename: String): MutableList<OutputRecord> {
return synchronized(lock) {
if (!files.containsKey(filename)) {
files[filename] = mutableListOf()
}
files[filename]!!
}
return files.getOrPut(filename) { mutableListOf() }
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@

package io.airbyte.cdk.mock_integration_test

import io.airbyte.cdk.command.ConfigurationJsonObjectBase
import io.airbyte.cdk.command.ConfigurationSpecification
import io.airbyte.cdk.command.DestinationConfiguration
import io.airbyte.cdk.command.DestinationConfigurationFactory
import io.micronaut.context.annotation.Factory
import jakarta.inject.Singleton

class MockDestinationConfiguration : DestinationConfiguration()

@Singleton class MockDestinationSpecification : ConfigurationJsonObjectBase()
@Singleton class MockDestinationSpecification : ConfigurationSpecification()

@Singleton
class MockDestinationConfigurationFactory :
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import javax.inject.Singleton

@Singleton
class MockDestinationWriter : DestinationWriter {
override fun getStreamLoader(stream: DestinationStream): StreamLoader {
override fun createStreamLoader(stream: DestinationStream): StreamLoader {
return MockStreamLoader(stream)
}
}
Expand Down