Update java_junit5_test to support a main_class attribute

[jeffmace]

Summary

Updates the java_junit5_test rule to support a main_class attribute and pass it on to java_test if provided. The default value remains unchanged.

Problem

The standard java_test has a default main_class but allows users to override the value with a different class. The functionality is not available for java_junit5_test. There is other functionality in the rule which is useful and should not be duplicated to a duplicate rule definition.

Solution

Instead of passing a hard-coded value for main_class, this change adds main_class as a part of the function definition so it can be optionally provided.

Testing

Added test cases for the default and overridden behavior under examples/tests_and_lints.

[mzeren-vmw]

This LGTM, and test works for me.

[shs96c]

The main thing that the java_junit5_test does is set the main_class. I'm not sure what the value of allowing that key detail to be overridden gives. Could you please explain a little more about the use case for this change?

[jeffmace]

The main thing that the java_junit5_test does is set the main_class. I'm not sure what the value of allowing that key detail to be overridden gives.

There's quite a lot of functionality in https://github.com/bazel-contrib/rules_jvm/blob/main/java/private/junit5.bzl related to system properties and jvm_flags.

Could you please explain a little more about the use case for this change?

In our case, we wrapped the default JUnit5Runner class with some additional functionality but still call JUnit5Runner at runtime. The java_test rule supports this interface so it made sense to update java_junit5_test to match it.

[jeffmace]

@shs96c - Could you take a look at the additional logic in java_junit5_test and see if this change is reasonable?

[mkosiba]

If you're changing main_class wouldn't you need to change the runtime_deps as well?

My $0.02 is that from a maintainability perspective I don't love adding a main_class argument as it's implying java_junit5_test now supports all sorts of runners, but that's not really the case. There is no formal API contract, there is no test coverage. If you subclass JUnit5Runner it'll likely work, if you don't it'll likely not work. So in effect we'd be adding an untested and effectively unsupported feature (unsupported in the sense that whenever rules_jvm changes in a way that breaks your use case, it won't be considered a bug/breaking change and you'll need to update your subclass/fork/etc.. to match).

The alternative is for you to copy the java_junit5_test macro into your repo, change the main class and use that. That use gives you pretty much all the guarantees you're already getting. The only difference is that with this PR, if the jvm_flags computation changes then you wouldn't need to update your fork.

This makes me think that perhaps moving the java_test kwargs computation into a helper function (that you can then use) or changing from java_test to a configurable factory function is a better way forward in this situation.