gradle: test task configuration changes #32108
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
octavia-squidington-iii
added
area/connectors
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
| testLogging { | ||
| events "passed", "skipped", "failed" | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| test { | ||
| testLogging { | ||
| // TODO: Remove this after debugging | ||
| showStandardStreams = true |
There was a problem hiding this comment.
I've made it so that we log everything all the time. If it's too verbose we can edit the log4j config instead.
| @@ -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 | |||
There was a problem hiding this comment.
This seems OK in practice, despite it being a reduction.
There was a problem hiding this comment.
those tests will definitely be IO bound. I'd bump that up to 50...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| } | ||
| integrationTestJava.configure { | ||
| mustRunAfter project.tasks.named('check') | ||
| dependsOn project.tasks.matching { it.name == 'assemble' } | ||
| } | ||
| project.tasks.named('build').configure { | ||
| dependsOn integrationTestJava | ||
| } |
There was a problem hiding this comment.
I made this task, and its python equivalent, required by build, because why not.
|
won't comment on the actual changes, but
🎉 this was one of the big wins on Destinations for our new TypingDeduping tests. If you want to copy any of that stuff, https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/java/airbyte-cdk/typing-deduping/src/testFixtures/java/io/airbyte/integrations/base/destination/typing_deduping/BaseTypingDedupingTest.java#L207-L232 is a decent place to start looking. (also, https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/java/airbyte-cdk/typing-deduping/src/testFixtures/java/io/airbyte/integrations/base/destination/typing_deduping/BaseTypingDedupingTest.java#L93 feels like a very prescient comment now 😅 ) |
|
Thanks! This validates my hunch that this approach is going to be worth it. |
|
I ran the connectors test separately for destination-snowflake, the only certified connector affected by this change, and they're ✅ except for the increment check which is fine to ignore: |
|
/approve-and-merge reason="skip CI because of superficial changes to many non-certified connectors" |
|
After a careful ML study, |
|
/approve-and-merge reason="skip CI because of superficial changes to many non-certified connectors. Previous failed because of added branch rules" |
|
/approve-and-merge reason="skip CI because of superficial changes to many non-certified connectors" |
|
This is really good |
octavia-approvington
deleted the
postamar/test-parallelism-and-logging
branch
This PR is a prereq to making source-postgres tests run concurrently in the same JVM to speed them up. What I want to do is define the testcontainer objects in static variables to amortize the cost of their creation, which in some cases is considerable.
Effectively, this PR removes the
numberThreadsandtestExecutionConcurrencygradle properties and replaces it withtestExecutionConcurrency:testExecutionConcurrencyis unset, gradle parallelizes the tests as best it can by spawning multiple JVMs. This is the default and it is what we already do.testExecutionConcurrencyis set to a positive integer, gradle only spawns one JVM and JUnit runs tests concurrently in as many threads as this property is set to.testExecutionConcurrencyis set to something else, it's the same as above but with as many threads as there are CPU cores.The idea here is in the future to set
testExecutionConcurrency=-1for source-postgres and rewrite the tests to be thread-safe (right now they aren't).This PR also renames the log4j config file in airbyte-commons to log4j2-test, to distinguish it from the other log4j config file in airbyte-cdk:core. I've noticed that in tests these two sometimes get mixed up. This way, the former (which is easier on the eyes) will always be used in tests.
Other quality-of-life changes have been noted via comments.