test perf, eliminate deploy jars from test classpath #391
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
plaird
commented
Jan 6, 2022
| @@ -299,6 +320,15 @@ public ParamFileResult findParamFilesForTestClassname(BazelWorkspace bazelWorksp | |||
| } | |||
|
|
|||
| for (String jarPath : jarPaths) { | |||
| if (!includeDeployJars && jarPath.endsWith("_deploy.jar")) { | |||
There was a problem hiding this comment.
This if statement is the key to this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Found the major culprit for #381
When Bazel builds a java_test target, it can in some cases produce two jars, the normal test jar (with just the compiled test classes) and sometimes a _deploy.jar which is a self contained runnable test executable. The deploy jar has all the deps built in.
For our internal test case, the tests each have 300+ dependencies, so the deploy jars were huge. When launching the tests from Eclipse, Eclipse spawns a new JVM and passes in the proper classpath via the -classpath arg. Also, the Eclipse test launcher is bundled inside. There is no need for the huge bloated deploy jars. So the fix in this PR is to prevent the deploy jars from being added to the test classpath.
Also, when running a whole suite of tests at the same time, we created a union classpath of all tests. That meant the test runner had 100 deploy jars built in (because there are 100 tests in this package).
Also reverted my change to use Sets to hold classpath elements. Classpaths are order sensitive. Packages should not have overlapping dependencies (two jars with the same class) but some packages don't follow best practices. Reverting to using Lists to maintain order is prudent.