Skip to content

Optimize junit dependencies for each test target #303

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

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Optimize junit dependencies for each test target #303

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions java/private/create_jvm_test_suite.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@honnix honnix Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes that seems like the case. I think the only thing that we may need from that list is data (if supplied)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does something like https://github.com/bazel-contrib/rules_jvm/compare/main...honnix:rules_jvm:filter-out-resources?expand=1 make sense to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it was like this since the beginning 7c008f2#diff-7e53ad4b884087fec5fdb0e9b2cf5d2f4fd2390dc68161d0a09878977a34414b

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#338 created PR, will see what the maintainers think of the approach.

name = deps_lib_name,
exports = deps,
visibility = ["//visibility:private"],
tags = tags,
testonly = True,
**library_attrs
)
runtime_deps_lib_name = "%s-test-runtime-deps-lib" % name
define_library(
name = runtime_deps_lib_name,
exports = runtime_deps,
visibility = ["//visibility:private"],
tags = tags,
testonly = True,
**library_attrs
)

for src in test_srcs:
suffix = src.rfind(".")
test_name = src[:suffix]
Expand All @@ -101,9 +121,9 @@ def create_jvm_test_suite(
size = size,
srcs = [src],
test_class = test_class,
deps = deps,
deps = [":" + deps_lib_name],
tags = tags,
runtime_deps = runtime_deps,
runtime_deps = [":" + runtime_deps_lib_name],
visibility = ["//visibility:private"],
**kwargs
)
Expand Down
Loading