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

gradle: test task configuration changes #32108

Merged
merged 7 commits into from
Nov 3, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
27 changes: 0 additions & 27 deletions airbyte-cdk/java/airbyte-cdk/config-models-oss/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -37,30 +37,3 @@ jsonSchema2Pojo {
tasks.register('generate').configure {
dependsOn tasks.named('generateJsonSchema2Pojo')
}

test {
useJUnitPlatform {
excludeTags 'log4j2-config', 'logger-client'
}
testLogging {
events "passed", "skipped", "failed"
}
}

tasks.register('log4j2IntegrationTest', Test) {
useJUnitPlatform {
includeTags 'log4j2-config'
}
testLogging {
events "passed", "skipped", "failed"
}
}

tasks.register('logClientsIntegrationTest', Test) {
useJUnitPlatform {
includeTags 'logger-client'
}
testLogging {
events "passed", "skipped", "failed"
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tasks were not doing anything. We don't have any of these junit tags defined anywhere. Presumably, this is a legacy from before the platform was spun out of this repo.

Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.core.LoggerContext;
import org.apache.logging.log4j.core.appender.OutputStreamAppender;
import org.apache.logging.log4j.core.config.Configuration;
import org.apache.logging.log4j.core.config.Configurator;
import org.apache.logging.log4j.core.config.LoggerConfig;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
Expand All @@ -43,14 +43,15 @@ public class AirbyteLogMessageTemplateTest {
public static final String CONSOLE_JSON_APPENDER = "ConsoleJSONAppender";
private static OutputStreamAppender outputStreamAppender;
private static LoggerConfig rootLoggerConfig;
private static LoggerContext loggerContext;

@BeforeAll
static void init() {
// We are creating a log appender with the same output pattern
// as the console json appender defined in this project's log4j2.xml file.
// We then attach this log appender with the LOGGER instance so that we can validate the logs
// produced by code and assert that it matches the expected format.
final LoggerContext loggerContext = (LoggerContext) LogManager.getContext(false);
loggerContext = Configurator.initialize(null, "log4j2.xml");
final Configuration configuration = loggerContext.getConfiguration();
rootLoggerConfig = configuration.getLoggerConfig("");

Expand All @@ -71,6 +72,7 @@ void setup() {
static void cleanUp() {
outputStreamAppender.stop();
rootLoggerConfig.removeAppender(OUTPUT_STREAM_APPENDER);
loggerContext.close();
}

@Test
Expand Down
8 changes: 0 additions & 8 deletions airbyte-cdk/java/airbyte-cdk/db-sources/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,6 @@ java {
}
}


test {
testLogging {
// TODO: Remove this after debugging
showStandardStreams = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made it so that we log everything all the time. If it's too verbose we can edit the log4j config instead.

}
}

project.configurations {
// From `base-debezium`:
testFixturesImplementation.extendsFrom implementation
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# currently limit the number of parallel threads until further investigation into the issues \
# where integration tests run into race conditions
numberThreads=1
testExecutionConcurrency=1
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# currently limit the number of parallel threads until further investigation into the issues \
# where integration tests run into race conditions
numberThreads=1
testExecutionConcurrency=1
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# currently limit the number of parallel threads until further investigation into the issues \
# where integration tests run into race conditions
numberThreads=1
testExecutionConcurrency=1
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# currently limit the number of parallel threads until further investigation into the issues \
# where integration tests run into race conditions
numberThreads=1
testExecutionConcurrency=1
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
# currently limit the number of parallel threads until further investigation into the issues \
# where Snowflake will fail to login using config credentials
numberThreads=4
parallelExecutionsPerThread=6
testExecutionConcurrency=4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems OK in practice, despite it being a reduction.

Copy link
Contributor

Choose a reason for hiding this comment

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

those tests will definitely be IO bound. I'd bump that up to 50...

Copy link
Contributor

Choose a reason for hiding this comment

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

there's some limit (either in the CI runner networking, or in the snowflake warehouse) that we run into beyond a certain parallelism limit. The right number might be somewhere in between :P

Copy link
Contributor

Choose a reason for hiding this comment

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

we probably sbhould look at https://docs.snowflake.com/en/sql-reference/parameters#max-concurrency-level and increase that number on the snowflake side, if we're too low there. Doesn't change the costs of running queries

71 changes: 40 additions & 31 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -505,27 +505,51 @@ subprojects { subproj ->
}

test {
maxHeapSize = '3g'
maxParallelForks = Runtime.runtime.availableProcessors()
// This allows to set up a `gradle.properties` file inside the connector folder to reduce parallelization.
// This is especially useful for connectors that share resources, like Redshift or Snowflake.
// This limits the number of test classes that execute in parallel.
// See also usages of parallelExecutionsPerThread.
if (subproj.hasProperty('numberThreads')) {
int numberThreads = 0
String numberThreadsString = subproj.property('numberThreads').toString()
if (numberThreadsString.isInteger()) {
numberThreads = numberThreadsString as int
}
if (numberThreads > 0 && numberThreads < maxParallelForks) {
maxParallelForks = numberThreads
}
useJUnitPlatform()
testLogging() {
events 'skipped', 'started', 'passed', 'failed'
exceptionFormat 'full'
showStandardStreams = true
}

// Set the timezone to UTC instead of picking up the host machine's timezone,
// which on a developer's laptop is more likely to be PST.
systemProperty 'user.timezone', 'UTC'

// Enable parallel test execution in JUnit by default.
// This is to support @Execution(ExecutionMode.CONCURRENT) annotations
// See https://junit.org/junit5/docs/current/user-guide/#writing-tests-parallel-execution for details.
systemProperty 'junit.jupiter.execution.parallel.enabled', 'true'
// Concurrency takes place at the class level.
systemProperty 'junit.jupiter.execution.parallel.mode.classes.default', 'concurrent'
// Within a class, the test methods are still run serially on the same thread.
systemProperty 'junit.jupiter.execution.parallel.mode.default', 'same_thread'
// Effectively disable JUnit concurrency by running tests in only one thread by default.
systemProperty 'junit.jupiter.execution.parallel.config.strategy', 'fixed'
systemProperty 'junit.jupiter.execution.parallel.config.fixed.parallelism', 1

if (!subproj.hasProperty('testExecutionConcurrency')) {
// By default, let gradle spawn as many independent workers as it wants.
maxParallelForks = Runtime.runtime.availableProcessors()
maxHeapSize = '3G'
} else {
// Otherwise, run tests within the same JVM.
// Let gradle spawn only one worker.
maxParallelForks = 1
maxHeapSize = '8G'
// Manage test execution concurrency in JUnit.
String concurrency = subproj.property('testExecutionConcurrency').toString()
if (concurrency.isInteger() && (concurrency as int) > 0) {
// Define a fixed number of threads when the property is set to a positive integer.
systemProperty 'junit.jupiter.execution.parallel.config.fixed.parallelism', concurrency
} else {
// Otherwise let JUnit manage the concurrency dynamically.
systemProperty 'junit.jupiter.execution.parallel.config.strategy', 'dynamic'
}
}

// Exclude all connector unit tests upon request.
if (rootProject.ext.skipSlowTests) {
// Exclude all connector unit tests
exclude '**/io/airbyte/integrations/source/**'
exclude '**/io/airbyte/integrations/destination/**'
}
Expand All @@ -534,15 +558,6 @@ subprojects { subproj ->
enabled = !rootProject.ext.skipSlowTests
excludes = ['**/*Test*', '**/generated*']
}
useJUnitPlatform {
excludeTags('cloud-storage')
}
testLogging() {
events "passed", "skipped", "failed"
exceptionFormat 'full'
// uncomment to get the full log output
// showStandardStreams = true
}
finalizedBy jacocoTestReportTask
}

Expand Down Expand Up @@ -587,8 +602,6 @@ subprojects { subproj ->
implementation libs.airbyte.protocol
}



tasks.withType(SpotBugsTask).configureEach {
// Reports can be found under each subproject in build/spotbugs/
reports {
Expand All @@ -598,9 +611,6 @@ subprojects { subproj ->
}

javadoc.options.addStringOption('Xdoclint:none', '-quiet')
tasks.named('check').configure {
dependsOn tasks.named('jacocoTestCoverageVerification')
}
}

// integration and performance test tasks per project
Expand All @@ -610,7 +620,6 @@ allprojects {
[
'integrationTestJava',
'integrationTestPython',
'standardSourceTestFile',
].contains(it.name)
}
}
Expand Down
35 changes: 10 additions & 25 deletions buildSrc/src/main/groovy/airbyte-integration-test-java.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -28,41 +28,26 @@ class AirbyteIntegrationTestJavaPlugin implements Plugin<Project> {
testClassesDirs = project.sourceSets.integrationTestJava.output.classesDirs
classpath += project.sourceSets.integrationTestJava.runtimeClasspath

useJUnitPlatform {
// todo (cgardens) - figure out how to de-dupe this exclusion with the one in build.gradle.
excludeTags 'log4j2-config', 'logger-client', 'cloud-storage'
}

useJUnitPlatform()
testLogging() {
events "passed", "failed", "started"
exceptionFormat "full"
// uncomment to get the full log output
// showStandardStreams = true
events 'skipped', 'started', 'passed', 'failed'
exceptionFormat 'full'
showStandardStreams = true
}

outputs.upToDateWhen { false }

systemProperties = project.test.systemProperties
maxParallelForks = project.test.maxParallelForks
maxHeapSize = project.test.maxHeapSize

systemProperties = [
// Allow tests to set @Execution(ExecutionMode.CONCURRENT)
'junit.jupiter.execution.parallel.enabled': 'true',
]

// Limit the number of concurrent tests within a single test class.
// See also usages of numberThreads.
if (project.hasProperty('parallelExecutionsPerThread')) {
int parallelExecutionsPerThread = project.property('parallelExecutionsPerThread').toString() as int
systemProperties = systemProperties + [
'junit.jupiter.execution.parallel.config.strategy': 'fixed',
'junit.jupiter.execution.parallel.config.fixed.parallelism': parallelExecutionsPerThread,
]
}
// Always re-run integration tests no matter what.
outputs.upToDateWhen { false }
}
integrationTestJava.configure {
mustRunAfter project.tasks.named('check')
dependsOn project.tasks.matching { it.name == 'assemble' }
}
project.tasks.named('build').configure {
dependsOn integrationTestJava
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this task, and its python equivalent, required by build, because why not.

}
}
8 changes: 8 additions & 0 deletions buildSrc/src/main/groovy/airbyte-python.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,14 @@ class AirbytePythonPlugin implements Plugin<Project> {
}

Helpers.addTestTaskIfTestFilesFound(project, 'integration_tests', 'integrationTestPython', installTestReqs)
def integrationTestTasks = project.tasks.matching { it.name == 'integrationTestPython' }
integrationTestTasks.configureEach {
dependsOn project.tasks.named('assemble')
mustRunAfter project.tasks.named('check')
}
project.tasks.named('build').configure {
dependsOn integrationTestTasks
}
}
}

Loading