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.

Sign up for GitHub

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

Jump to bottom

[F61iNGQX] Splits integration tests into their own package #326

Merged
merged 6 commits into from
Feb 16, 2023
Merged

[F61iNGQX] Splits integration tests into their own package #326

merged 6 commits into from
Feb 16, 2023

Conversation

ncordon
Copy link
Collaborator

@ncordon ncordon commented Feb 9, 2023 •
edited

What

  • Splits integration tests into a separate package it.
  • Executes the projects in parallel (org.gradle.parallel=true in gradle.properties file).
  • Chunks the CI into compile + each of subprojects tests vs just a job doing compilation + tests.

Why

Because:

  • That way we can actually run the tests in parallel.
  • We halved the time of execution for GitHub Actions, TeamCity and local testing.
  • We can detect what project was having test failures better, or wether the failure happened in compilation instead.

GitHub Actions

The CI projects have been split and refactored, we are down from ~30 min to ~16 min

TeamCity

By using gradle parallelism we are down from ~30 min to 16min as well

Locally

Because I have more CPU processors in my laptop than the CI (2 in GitHub Actions, for example), now you can run core:tests (used to take ~30 min locally) in roughly 5 min. The it-tests is a different story, because they require lots of docker containers, so they are still very slow and we cannot parallelise them because some of the schema tests there would break (already tried that).

Screenshot 2023-02-10 at 14 25 35

@ncordon ncordon force-pushed the dev-split-it-tests branch 5 times, most recently from d5ae877 to a460975 Compare February 9, 2023 10:09
@ncordon ncordon changed the title Splits integration tests into their own package [NOID] Splits integration tests into their own package Feb 9, 2023
@ncordon ncordon changed the title [NOID] Splits integration tests into their own package [F61iNGQX] Splits integration tests into their own package Feb 9, 2023
@ncordon ncordon added the NOT READY FOR MERGE PR isn't ready to be merged label Feb 9, 2023
@ncordon ncordon force-pushed the dev-split-it-tests branch 12 times, most recently from 26d6509 to 2d5aff5 Compare February 10, 2023 11:13
@ncordon
[F61iNGQX] Splits the integration tests into a separate package
2d5aff5 
[F61iNGQX] Makes the test run in parallel

[F61iNGQX] Tries to run projects in parallel

Would printing less spead up the builds?

Tries to run core in parallel

Ignores AtomicTest

Uses temurin as JVM and decreases the heap size

Tries overriding the gradle.properties

Source
actions/runner-images#6709
gradle/gradle#19750

Tries to fix number of threads to 4

Adds separate ci step

Adds separate step in CI

Downloads docker containers
@ncordon ncordon force-pushed the dev-split-it-tests branch 8 times, most recently from ce65b54 to 6bc6728 Compare February 10, 2023 11:54
@ncordon ncordon force-pushed the dev-split-it-tests branch 2 times, most recently from a89db83 to 8881a97 Compare February 10, 2023 13:38
@ncordon
[F61iNGQX] Reuses testing project
f242cec 
[F61iNGQX] Splits Snyk tests into separate file
ncordon
ncordon commented Feb 10, 2023
View reviewed changes
build.gradle
@@ -75,7 +75,7 @@ subprojects {
'neo4jCommunityDockerImage': System.getProperty("NEO4JVERSION") ? 'neo4j:' + System.getProperty("NEO4JVERSION") : 'neo4j:5.6.0',
'coreDir': 'core'

maxHeapSize = "8G"
maxHeapSize = "5G"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure this had any impact, but GitHub Actions machines have 6GB of memory, so if this was honoured it could be problematic

Copy link
Collaborator Author

@ncordon ncordon Feb 16, 2023 •
edited

Choose a reason for hiding this comment

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

512m to run the tests is too little I believe. Possibly the settings in 4.4 should be changed, not these ones. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes my bad, I didn't explain very well :/

I meant to put an if (System.env.CI != null) { ... },

For example, instead of decrease the maxHeapSize also locally, maybe we could do:

maxHeapSize = "8G"

// This would apply only to GitHub Actions
if (System.env.CI != null) {
   maxHeapSize = "5G"
}

given that the minHeapSize = "128m" is already present (line 82).
Wdyt?

Anyway, I totally agree the 4.4 maxHeapSize is too low,
so in any case I would make it consistent with the 5.x one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah now I get your point. Don't you think anyway having a consistent setup for local and CI runs (like having the same heap space) could be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah well, thinking about it maybe it's not the best to have 2 different setups.
That's okay then :)

ncordon
ncordon commented Feb 10, 2023
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
@ncordon ncordon added team-cypher-surface dev and removed NOT READY FOR MERGE PR isn't ready to be merged labels Feb 10, 2023
vga91
vga91 reviewed Feb 15, 2023
View reviewed changes
.github/workflows/CI.yaml Outdated Show resolved Hide resolved
vga91
vga91 reviewed Feb 15, 2023
View reviewed changes
Copy link
Collaborator

@vga91 vga91 left a comment

Choose a reason for hiding this comment

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

Really nice work!

I would suggest just a few small changes.

Also I'm wondering, how come some it-test schemas don't work in parallel?
It would be nice to parallelize them, if feasible, since as you said, they are very slow.

core/build.gradle Outdated Show resolved Hide resolved
it/build.gradle Outdated
testImplementation project(':core')
testImplementation project(':test-utils')
testImplementation project(":common").sourceSets.test.output
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that this line is not needed,
only sourceSets.test.output of core are imported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't directly for the code. We do for the test resources:

it/src/test/java/apoc/it/common/UtilIT.java Outdated Show resolved Hide resolved
@ncordon ncordon force-pushed the dev-split-it-tests branch 5 times, most recently from e46a1c8 to 086d686 Compare February 16, 2023 09:38
vga91
vga91 approved these changes Feb 16, 2023
View reviewed changes
Copy link
Collaborator

@vga91 vga91 left a comment

Choose a reason for hiding this comment

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

LGTM, really great job!

@ncordon ncordon merged commit bcddf61 into dev Feb 16, 2023
@ncordon ncordon deleted the dev-split-it-tests branch February 16, 2023 16:31
@ncordon ncordon mentioned this pull request Feb 24, 2023
Merged
@ncordon ncordon mentioned this pull request Jun 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vga91 vga91 vga91 approved these changes

Assignees

@vga91 vga91

Labels
dev team-cypher-surface
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ncordon @vga91