Optimize junit dependencies for each test target #303
Conversation
|
Thanks @shs96c for taking a look! Is there plans to publish a release in the near future? |
|
@shs96c would a release be possible, or is there one planned in the near future? |
|
We can push a release, but I'd like us to fix the artifact hashing algorithms first. My day job has been pretty busy, so I've not had much time to focus on that. |
|
Totally understandable. Ill update us to use a commit in the mean time. Thanks for the update! |
| @@ -91,6 +91,26 @@ def create_jvm_test_suite( | |||
|
|
|||
| tests = [] | |||
|
|
|||
| # Optimization for classpath, reduces the duplicate dependencies for each test to instead rely on one target | |||
| deps_lib_name = "%s-test-deps-lib" % name | |||
| define_library( | |||
There was a problem hiding this comment.
Not entirely sure but will these two new jars include test resources? We are seeing an issue when using liquibase in a test, as it complains about duplicated resources files from different jars.
There was a problem hiding this comment.
It will not create any jar as there are no sources to this library. It should only be a bazel construct which allows aspects to simplify the class path resolution.
There was a problem hiding this comment.
Hmm, that's not what we observe. Yes it doesn't contain any Java source code, but it contains test resources. Could it be https://github.com/brian-mcnamara/rules_jvm/blob/d899e276ff1a4a9b98272ed6cd31991f09d7839a/java/private/create_jvm_test_suite.bzl#L16? data might also be troublesome.
There was a problem hiding this comment.
Ah yes that seems like the case. I think the only thing that we may need from that list is data (if supplied)
There was a problem hiding this comment.
Does something like https://github.com/bazel-contrib/rules_jvm/compare/main...honnix:rules_jvm:filter-out-resources?expand=1 make sense to you?
There was a problem hiding this comment.
Sorry you are right that we do not have nontest_srcs for those cases, and it was indeed the case that resources would be duplicated in both -test-lib and the test jars, before your change.
There was a problem hiding this comment.
That said, I think it is still not ideal having things duplicated. Maybe resources should not be included in -test-lib jar (I don't know the reason why they are explicitly included).
There was a problem hiding this comment.
Looks like it was like this since the beginning 7c008f2#diff-7e53ad4b884087fec5fdb0e9b2cf5d2f4fd2390dc68161d0a09878977a34414b
There was a problem hiding this comment.
Ooph, not great, but thanks for confirming my test was not doing something crazy. I agree it does not make sense to have resources included multiple times. Let me at least propose a PR and see where it goes :)
There was a problem hiding this comment.
#338 created PR, will see what the maintainers think of the approach.
This change simplifies the dependency list for the junit suite test targets by creating a export-only library used to group the dependencies together via an export-only java_library. The primary reason for this is to simplify the project model for IntelliJ using the new Bazel plugin. With this change, the IntelliJ model does not duplicate the dependencies for each test class in the Aspects and the internal project model and instead only refers to the java library.