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

Increase test processes to start in parallel #2953

Merged
merged 4 commits into from Apr 26, 2023

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Mar 31, 2023

@@ -22,6 +22,12 @@ java {
tasks.withType<Test>().configureEach {
useJUnitPlatform()

maxParallelForks = if (System.getenv("CI") != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can read dokka_integration_test_parallelism like

project.properties["dokka_integration_test_parallelism"]?.toString()?.toIntOrNull()?.let { parallelism ->
maxParallelForks = parallelism
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are more cores in TeamCity runners, maybe we can use

    maxParallelForks = if (System.getenv("GITHUB_ACTIONS") != null) {
        Runtime.getRuntime().availableProcessors()
    } else {
        (Runtime.getRuntime().availableProcessors() / 2).takeIf { it > 0 } ?: 1
    }

for both tests and integrationTests, get rid of dokka_integration_test_parallelism property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Goooler Goooler changed the title Execute tests in parallel Increase test processes to start in parallel Mar 31, 2023
@IgnatBeresnev IgnatBeresnev self-requested a review April 6, 2023 22:19
@IgnatBeresnev
Copy link
Member

IgnatBeresnev commented Apr 26, 2023

Finally set out some time to test it.

I think it makes perfect sense to do this for unit tests, absolutely no questions here. But I wasn't sure what sort of an impact it would have on our integration tests.

I have a totally overspec'd workstation (16C, 32T, 128GB RAM) to draw any conclusions, but running the integrationTest task multiple times with caching disabled:

  • With maxParallelForks = 2 it takes ~17 minutes, max RAM consumed throughout each run is ~20GB
  • With maxParallelForks = 16 (i.e with changes from this PR) it takes ~10 minutes, max RAM consumed throughout each run is ~50GB.

I actually wonder what would happen if I didn't have enough memory - would the integration tests fail, or would the time increase drastically because of GC problems?

I'm trying to trigger integration tests on teamcity for this PR, but teamcity is not picking up the branch changes like it did before... Our admins might've introduced some security policies, so I'll have to resolve that first, which might take time...

I'd be happy to merge this change for unit tests only though, if you open a separate PR or change this one

@IgnatBeresnev
Copy link
Member

(looks like TeamCity was simply lagging yesterday, integration tests work now)

Copy link
Member

@IgnatBeresnev IgnatBeresnev left a comment

Choose a reason for hiding this comment

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

Thank you!

I'm still interested in having (or at least testing) this change for integration tests, so @Goooler if you want and have the time, feel free to open a separate PR :)

@IgnatBeresnev IgnatBeresnev merged commit 43c33f6 into Kotlin:master Apr 26, 2023
10 of 12 checks passed
@Goooler Goooler deleted the test_in_parallel branch April 26, 2023 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants