java_test: make it work for multiple junit tests and non tests sources without Skylark wrapper and dependent library

[davido]

java_test rule doesn't work as I would expect it should. I would expect to write this and provide mixed Junit tests and utility (also not tests classes) to the srcs glob.

Buck version:

  java_test(
    name = 'evict_cache_tests',
    srcs = glob(['src/test/java/**/*.java']),
  )

This doesn't work. Instead, I need this Skylark wrapper to generate JUnit Suites for me: [1]. Even then, it doesn't work because I must separate the utility library with non test sources from the JUnit test sources.

Bazel version:

load("//tools/bzl:junit.bzl", "junit_tests")

java_library(
    name = "evict_test_utills",
    srcs = glob([<non tests utility classes only>]),
)

junit_tests(
    name = "evict_cache_tests",
    srcs = glob(["src/test/java/**/*Test.java"]),# this would only pick test sources
    deps = [":evict_test_utills"],
)

[1] https://github.com/GerritCodeReview/gerrit/blob/master/tools/bzl/junit.bzl

[damienmg]

Reassigning to @kush-c who want to remove --legacy_bazel_java_test since this should be a blocker for the flag removal.

[kush-c]

Actually I was just trying to clean up code, as it thought it was deprecated. Assigning this back to you @iirina since I didn't anticipate this blocker, and I may not be working on this anytime soon.

[iirina]

Sorry, the bot misinterpreted the commit message. Reopening this.

[ittaiz]

I have something working in my fork of the scala rules and I'm trying to see if/how it can be generalized to being it's own rule.
My general approach is to support the stock Maven features of running all junit classes which adhere to a prefix.
Basically I just list the entries of the archive, grep the relevant ones and output to a file. Later on in my custom runner I read the names of classes from this file and pass them to JUnit.
My big question marks are:

1. How to use the data returned from the rule discover_classes? In my working implementation this is part of a rule so no problem with accessing the struct members but when trying to extract this to be a standalone rule then I get into the problem where to support java I have to use a macro and it's unclear if/how to use that info in the macro. I think providers are my friends but I don't know how to use them in the macro.
2. How to split the kwargs and give java_library what it needs and java_test what it needs.

def _impl(ctx):
    ctx.action(
      inputs=[archive],
      outputs=[ctx.outputs.classes],
      progress_message="Discovering classes with suffix of Test",
      command="unzip -l {archive} | grep Test.class > {out}".format(archive=archive.path, out=ctx.outputs.classes.path))
    return struct(suite_class = "io.bazel.rulesscala.test_discovery.DiscoveredTestSuite", classesFlag = "-Dbazel.discovered.classes.file.path=%s" % ctx.outputs.classes.short_path)


discover_classes = rule(
  implementation=_impl,
  attrs={
      "archive": attr.label(single_file=True, allow_files=True),
      "_suite": attr.label(default=Label("//discover_tests/support/io/bazel/test_discovery:test_discovery"), single_file=True, allow_files=True),
      },
  outputs={
      "classes": "%{name}_discovered_classes",
      },
)

def java_unit_test(name, srcs, **kwargs):
    #pass **kwargs but filter only things that are relevant to java_library
    native.java_library(name = name + "_java_lib", srcs = srcs)
    discovery = discover_classes(name = name +"_discover_tests", archive = name + "_java_lib")
    runtime_deps = [name + "_java_lib"] + kwargs.get("runtime_deps",[])
    jvm_flags = [discovery.classesFlag] + kwargs.get("jvm_flags",[])
    data = [name +"_discover_tests"] + kwargs.get("data",[])
    return native.java_test(name = name, runtime_deps= runtime_deps, test_class= discovery.suite_class, jvm_flags = jvm_flags, data = data)

[ittaiz]

@damienmg following our conversation I'll wait your update if you think this can be done outside of the team, right? If so I'll try to find volunteers.

[steren]

@ittaiz that works we would be happy to review your changes

[pgr0ss]

Is there an update on this issue? Either a built in way in bazel, or an official skylark wrapper to generate test suites?

[ittaiz]

I have a solution in rules_scala which I want to pull out into a generic solution. It emulates maven's behavior of scanning the classes of the current target's jar and runs all which fit the input prefixes/suffixes. I think it's blocked though on having location expansion support runtime.

…

On Fri, 15 Sep 2017 at 19:47 Paul Gross ***@***.***> wrote: Is there an update on this issue? Either a built in way in bazel, or an official skylark wrapper to generate test suites? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFz2wvaQzjieFhSmPoSH3qSA__Xw4ks5siqoigaJpZM4MDLq-> .

[davido]

Gerrit Code Review project extracted Bazlets repository. You can use its junit_tests Skylark rule.

https://github.com/GerritCodeReview/bazlets/blob/master/tools/junit.bzl#L65-L73

[carl-mastrangelo]

Voicing my support of this. Using bazel after coming from Gradle feels like 2 steps forward 1 step back. It's debilitating trying to find what the "correct" way to do Java testing is in Bazel.

If possible, could this be raised to P2? Lack of testing support is the #1 blocker for me moving to Bazel.

[ittaiz]

@carl-mastrangelo thanks to mild pressure by @natansil we did some initial work to extract this mechanism from rules_scala.
It's ongoing very slowly but if you want to take a look at the branch and say if this will help you maybe it will add motivation to the effort

[mleonhard]

How about also providing some guidance on testing in Migrating from Maven to Bazel? That is the first place everyone looks.

[mleonhard]

I would like to add that the suggestion to explicitly define tests in an AllTests.java file is super risky. I would add a test, run it in my editor, and then forget to add it to AllTests.java. Then the test will never get run again. This is an error-prone software engineering process.

[mleonhard]

An easy workaround is to add this to all non-test classes:

/** Workaround for https://github.com/bazelbuild/bazel/issues/2539 */
@Test
public void emptyTest() {}

[OkJaybird]

I wanted to link to @davido's very helpful comment in #10036, since this issue seems to be where active discussion on this feature request is taking place. While I had previously been using junit_tests from https://gerrit.googlesource.com/bazlets, I found it did not play well with IDEs when wanting to write and run tests as one-offs, because the underlying java_test directives did not exist. The gen_java_tests macro provided the behavior I expected & was hoping for, both in IDE and CLI runs.

# WORKSPACE
git_repository(
    name = "google_bazel_common",
    commit = "6db2b87d28f765f0064bc437bd489b432ec101b8",
    remote = "git://github.com/google/bazel-common.git",
)
load("@google_bazel_common//:workspace_defs.bzl", "google_common_workspace_rules")
google_common_workspace_rules()

# BUILD
java_library (
    name = "lib_src",
    srcs = glob(["src/main/java/**/*.java"]),
    resources = glob(["src/main/resources/**"]),
    deps = DEPS
)

java_library (
    name = "lib_test",
    srcs = glob(["src/test/java/**/*.java"]),
    deps = [":lib_src"] + DEPS_TEST
)

gen_java_tests(
    name = "test_all",
    srcs = glob(["src/test/java/**/*Test.java"]),
    deps = [
        ":lib_src",
        ":lib_test",
    ],
)

Hopefully this helps someone looking for a viable workaround!

[wiwa]

Is anyone working on this?

[swarren12]

Just to add another voice in support of this feature, it would prove very useful for large codebases with many tests, especially when porting from a system like Buck that already supports this.

Specifically, there are two "workarounds" that currently show up in conversations about this, but both have downsides:

1. Use a macro wrapper to turn the glob of source files into distinct java_test targets - this "works", but as is documented elsewhere it comes with a significant performance impact due to having to create and tear-down JVMs.
2. Generate a @Suite (either at compile time or dynamically via something like AllTests) - this clashes with --flaky_test_attempts as the suite is detected as having failed and so all tests are run again. This can be somewhat mitigated by sharding the AllTests, but that then relies on tests being parallisable (okay for unit, less so for higher-level tests, which also tend to be slower and so suffer more from this problem!)

Looking through the source code, I saw that BazelTestRunner ultimately creates a request by delegating to JUnit's Request::aClass. Since there is also Request::classes, I had a go at specing out a patch to BazelTestRunner that makes use of this.

swarren12@8e090ee

This is entirely a PoC, but thought I would share it to elicit some feedback on if this is a viable approach and, if not, why?

Some obvious improvements are allowing tests to be passed in without having to split on ; or possibly building in classpath detection and allowing a pattern to be specified instead. 🤷‍♂️

Also, wasn't quite sure how to deal with the fact the current implementation expects a single name for the suite; currently just cobbled something in but a better solution might be to pass in the name of the java_test rule or something and use that. Again, it's a PoC so I didn't spend too much time thinking about it!

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

[swarren12]

It would be nice to know what the current state of this is, as it is still an issue; however, I'm wondering if it would make sense to close this one and open a new issue under rules_java instead as (my understanding is that) that project is the future for the Java rules?