dedupe the list of jar files from test param files #388
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 4, 2022
| for (String jarPath : jarPaths) { | ||
| // it is important to use a Set for allPaths, as the jarPaths will contain many dupes | ||
| // across ParamFiles and we only want each one listed once | ||
| allPaths.add(jarPath); |
There was a problem hiding this comment.
Previously, we were aggregating all the jar paths using a List not a Set.
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.
There is a pause in between the time the user first launches the JUnit tests, and when the tests actually start running. For smaller packages this isn't that noticeable. But for larger projects this is a big problem. This is a partial fix for #381
One package internally has tests in 100 test classes, and the pause to launch them before this change was 6 minutes. This PR reduces that by 25%.
The fix here is to dedupe the list of jar files that come from the Bazel param files (e.g. src/test/java/com/foo/apple/AppleTest_deploy.jar-0.params) which list the classpath for each test class. The internal package has 300 jars on classpath for each test classpath, so the total set of jars we were returning was 30,000 (300x100). Since the classpath was identical for each test class, this fix now correctly returns just 300 jar files as the combined classpath.
Why writing the test for this, I found I had to refactor to make it testable. I discovered that the core logic could be moved from BEF Core to the Bazel SDK.