Skip to content

Conversation

@asinbow
Copy link

@asinbow asinbow commented Jul 21, 2020

Overview

We are developers from Flexport. This PR is to open source how we solve this issue(bazelbuild/bazel#6681), and make Bazel support JUnit5.

A blog post talks about this: https://flexport.engineering/connecting-bazel-and-junit5-by-transforming-arguments-46440c6ea068


I hereby agree to the terms of the JUnit Contributor License Agreement.

@asinbow
Copy link
Author

asinbow commented Jul 23, 2020

@hcoona Could you help me review this PR?

@asinbow asinbow changed the title open source BazelJUnit5ConsoleLauncher Connecting Bazel and JUnit5 by BazelJUnit5ConsoleLauncher Jul 23, 2020
@razfriman
Copy link

You're a hero @asinbow - Would love to see this get merged!

@razfriman
Copy link

  1. AWESOME work! I was able to integrate this succesfully and it works.

  2. Do you have this issue where it prints out all the correct test output results to the console, but does not show you the details of which test ran?

What I see:

Screen Shot 2020-07-23 at 4 32 46 pm

What I'd like to see: (based on https://blog.jetbrains.com/idea/2016/08/using-junit-5-in-intellij-idea/)

idea-5Nested

@marcphilipp
Copy link
Member

Interesting workaround! Is there a way in Bazel (excuse my ignorance) to reuse the BazelJUnit5ConsoleLauncher without having to copy it around?

@asinbow
Copy link
Author

asinbow commented Jul 24, 2020

Interesting workaround! Is there a way in Bazel (excuse my ignorance) to reuse the BazelJUnit5ConsoleLauncher without having to copy it around?

Maybe we can move this into a standalone repo, and publish a jar to a maven repo?

@asinbow
Copy link
Author

asinbow commented Jul 25, 2020

  1. Do you have this issue where it prints out all the correct test output results to the console, but does not show you the details of which test ran?

Yep, it's empty.
image

It seems XML_OUTPUT_FILE is not transformed for JUnit5, and there are more environments that should be transformed.
image

Do you know how set the file path of XML report for JUnit5? I could not find it here: https://junit.org/junit5/docs/current/user-guide/#running-tests-console-launcher-options

@marcphilipp
Copy link
Member

Do you know how set the file path of XML report for JUnit5? I could not find it here: junit.org/junit5/docs/current/user-guide/#running-tests-console-launcher-options

Is --reports-dir what you're looking for? The ConsoleLauncher will write one file per test engine into that directory.

@asinbow
Copy link
Author

asinbow commented Jul 25, 2020

@marcphilipp 👍
JUnit4 passes in XML_OUTPUT_FILE=<reports-dir>/test.xml as the XML report file path, while JUnit5 receives --report-dir as the XML report file directory, and create a file like TEST-junit-jupiter.xml there. https://github.com/junit-team/junit5/blob/37e0f559277f0065f8057cc465a1e8eb91563af6/junit-platform-reporting/src/main/java/org/junit/platform/reporting/legacy/xml/LegacyXmlReportGeneratingListener.java#L116

@razfriman I think I have already fixed this issue.
image
The source code will be reviewed internally first, so you have to wait for days.

BTW, the "Rerun Failed Tests" doesn't work at all, if we can fix this, it would boost our productivity hugely.
Do you guys have any ideas about it?
image

https://github.com/bazelbuild/intellij/blob/194f34b0690827918356eab5ae84fe85c8f6c50c/base/src/com/google/idea/blaze/base/run/smrunner/BlazeRerunFailedTestsAction.java#L89

private static void fixXmlOutputFile(String xmlOutputFile) {
if (xmlOutputFile != null && !xmlOutputFile.isEmpty()) {
Path shouldPath = Paths.get(xmlOutputFile);
Path realPath = shouldPath.getParent().resolve(DEFAULT_JUNIT_JUPITER_XML_OUTPUT_FILE);
Copy link
Member

Choose a reason for hiding this comment

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

I guess you'd need a command to merge multiple files when there are multiple test engines... 🤔

Copy link
Author

@asinbow asinbow Jul 30, 2020

Choose a reason for hiding this comment

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

Yep, you are right, I found this issue after pushing the commit as well 🤣. Will have a try.

Copy link
Member

Choose a reason for hiding this comment

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

We could add an option to the ConsoleLauncher to write a single xml report file to support this use case. Could you please open an issue for that on the junit5 repo?

Copy link
Author

@asinbow asinbow Aug 2, 2020

Choose a reason for hiding this comment

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

I did some tests, and find that no matter how many tests in one running, it will only generate a single XML file named like TEST-*.xml, junit5 still does well, so I think we can simply move it to test.xml.
BTW, junit5 marks the XML report file as legacy: https://github.com/junit-team/junit5/blob/main/junit-platform-reporting/src/main/java/org/junit/platform/reporting/legacy/xml/LegacyXmlReportGeneratingListener.java
I am not familiar with junit5, and feedback is welcome.

Choose a reason for hiding this comment

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

I found multiple XML files generated when I mixed both vintage tests and JUnit 5 Jupiter tests.

Would be great to have the option to generate a single file as @marcphilipp suggwsted

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, interesting. Do you guys have a demo project for multiple XML files?

@asinbow asinbow force-pushed the open-source-BazelJUnit5ConsoleLauncher branch from de01676 to 06c3780 Compare August 1, 2020 09:32
@ctasada
Copy link

ctasada commented Aug 14, 2020

@asinbow I'm really looking forward to see this merged. In the meantime I did a few tests and I still found some issues :(

Checking JUnit 5 Guide I see that select-package is documented as

-p, --select-package=PKG Select a package for test discovery. This option can be
repeated.

In one of my Java modules I have 2 packages, so I need to pass this parameter more than once to work properly, so something like:

    if test_package:
        for pkg in test_package:
            junit_console_args += ["--select-package", pkg]
    else:
        fail("must specify 'test_package'")

would be needed.

Also, due to constraints on some of my test cases, I have modules that run both jupiter and vintage tests, but only the TEST-junit-jupiter.xml file is copied.

Finally, but not last, while running in IntelliJ 2020.1 with Bazel plugin 2020.07.13.0.2 (latest available)
image
Same for failing tests
image

I see a previous comment where you talk about it, but doesn't seem to be working for me. Also not sure if it's an issue with the Bazel plugin or the JUnit5 rule

Thanks for the amazing work

@asinbow
Copy link
Author

asinbow commented Aug 21, 2020

@ctasada Thanks for your feedback with these use cases.

Checking JUnit 5 Guide I see that select-package is documented as --select-package

There is a bit weird logic to erase --select-package, let me think about your case.

Also, due to constraints on some of my test cases, I have modules that run both jupiter and vintage tests, but only the TEST-junit-jupiter.xml file is copied.

I am afraid in this case we have to merge these test reports together into a single file.

@cthornton
Copy link

I looked into merging multiple JUnit reports. If you're willing to pull in ant as a dependency (ant and junit-ant), they have some code you can leverage. I ended up writing the following code:

 private static File combineJunitReports(File reportDirectory, File[] junitReports) {
    File output = new File(reportDirectory, "junit-combined.xml");

    Project project = new Project();
    project.setName(JUnit5Launcher.class.getName());
    project.setBaseDir(reportDirectory);

    XMLResultAggregator aggregator = new XMLResultAggregator();

    for (File report : junitReports) {
      FileSet fileSet = new FileSet();
      fileSet.setProject(project);
      fileSet.setFile(report);
      aggregator.addFileSet(fileSet);
    }

    aggregator.setProject(project);
    aggregator.setTodir(reportDirectory);
    aggregator.setTofile(output.getName());
    aggregator.execute();

    return output;
  }

And modified the existing code in this pr like so:

  File outputFile;
    if (files.length > 1) {
      outputFile = combineJunitReports(dir.toFile(), files);
    } else {
      outputFile = files[0];
    }

    try {
      Files.move(outputFile.toPath(), requiredPath);
    } catch (IOException e) {
      e.printStackTrace();
    }

@ponnapz
Copy link

ponnapz commented Sep 2, 2020

@asinbow Great work on this and thanks for sharing! I gave this a try on our project and it mostly works. Some current shortcomings are:

  1. Only displays test results for JUnit Jupiter and not vintage as already discussed above.
  2. Jump to source by clicking on the tests doesn't work
  3. Re-running only failed tests doesn't work

I took a stab at fixing these:

  1. Merge test result xmls from JUnit Jupiter and Vintage to get all test results. @marcphilipp's suggestion to add an option to ConsoleLauncher to generate a single file would make this easier.
  2. Looks like IntelliJ expects the name attribute in <testcase> to only have the method name without parentheses etc. Modifying the name fixed 'jump to source'
  3. I did some hacky fixes to parse TESTBRIDGE_TEST_ONLY correctly especially when re-running failed tests from multiple classes. But there are still issues as the bazel IntelliJ plugin does have some issues with setting the --test_filter param correctly (https://github.com/bazelbuild/intellij/search?q=--test_filter&type=Issues). For example this case makes it a bit difficult to parse:
--test_filter=com.example.project.CalculatorTests#add,add,add,add,com.flexport.bazeljunit5.BazelJUnit5ConsoleLauncherTest#filterOptions,filterOptions,filterOptions,filterOptions,filterOptions,filterOptions,filterOptions,filterOptions,parseOptions,parseOptions,parseOptions,parseOptions,parseOptions,parseOptions

I also tried to improve how @ParameterizedTest are reported by including the parameter values for each run in the display name. Works in some cases.
image

You can find my changes here - asinbow#1

def java_junit5_test(name, srcs, test_package, deps = [], runtime_deps = [], **kwargs):
def java_junit5_test(name, srcs, test_package, main_class = None, deps = [], runtime_deps = [], **kwargs):
FILTER_KWARGS = [
"main_class",

Choose a reason for hiding this comment

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

this line can be removed if there's a main_class argument

}

try {
Class.forName(testBridgeTestOnly);

Choose a reason for hiding this comment

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

--test_filter=package.class.method is also valid. In fact, my IDE, IntelliJ IDEA 2020.2, is automatically generating this form for Kotlin test code.

Probably need more logic to handle this

Copy link

@rdesgroppes rdesgroppes Jan 20, 2022

Choose a reason for hiding this comment

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

Hum, when started from within IntelliJ IDEA, this doesn't seem to work for nested classes: bazelbuild/intellij#245


try {
Document mergedXmlOutput = mergeTestResultXmls(files);
writeXmlOutputToFile(mergedXmlOutput, requiredPath.toString());

Choose a reason for hiding this comment

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

Is requiredPath.toString() equals to xmlOutputFile?

@cthornton
Copy link

I found some issues with parsing the test filter, especially with @Nested tests, and running multiple tests. Here's a solution I made here in this gist: https://gist.github.com/cthornton/f21caeebbc087a9701aaf5ac1c39f5ce

Apologies for not having time to make a PR to this branch

@Jonpez2
Copy link

Jonpez2 commented Nov 12, 2021

Hello! This looks amazing. Is it likely to get merged in?

Thank you all for the work!

@marcphilipp
Copy link
Member

@asinbow I think it would be good to provide this interim solution to Bazel users in a standalone repo and potentially publish the required code to Maven Central so there's less copy-and-paste involved. However, the JUnit team does not use Bazel and is therefore ill-suited to maintain that. I can imagine hosting it under our org but only if you or someone else that's interested make a commitment to maintain it. WDYT?

@wjdix
Copy link

wjdix commented Nov 19, 2021

Hey y'all I have been trying to adapt this into my project (I translated the code into Kotlin) and have noticed that Intellij uses the following format specifying a focused test for a single method: <package>.<class>.<method>, i.e. org.example.FooTest.testA. The implementation here does not handle that format. I extended my local version as follows:

            val className = mutableTestBridgeTestOnly.split(".").dropLast(1).joinToString(".")
            val methodName = mutableTestBridgeTestOnly.split(".").lastOrNull()
            methodName?.let {
                val klass = try {
                    Class.forName(className)
                } catch (e: ClassNotFoundException) {
                    throw IllegalStateException(e)
                }
                return listOf("$SELECT_METHOD=" + ReflectionUtils
                    .getFullyQualifiedMethodName(klass, methodName))
            }

Just an FYI that there may be a missing a case to handle here.

@marcphilipp
Copy link
Member

How does this PR relate to the new java_junit5_test rule?


if (testBridgeTestOnly.contains("#")) {
// There could be multiple classes in TESTBRIDGE_TEST_ONLY which are separated by $|
String[] splits = testBridgeTestOnly.split("\\$\\|");
Copy link

@rdesgroppes rdesgroppes Jan 20, 2022

Choose a reason for hiding this comment

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

#| is also a separator for classes without explicit methods, s.a. in: my.pkg.TestClass1#|my.pkg.TestClass2#

@marcphilipp
Copy link
Member

How does this PR relate to the new java_junit5_test rule?

Anyone? I feel like I'm talking to myself here. 😕

@guyboltonking
Copy link

How does this PR relate to the new java_junit5_test rule?

Anyone? I feel like I'm talking to myself here. 😕

We're looking at using java_junit5_test; so far I have a test project that works as expected, and I intend to roll it out to a larger monorepo in the near future. First impressions: it appears to be a workable solution, and it definitely integrates well with the Bazel IntelliJ plugin, providing individual test filtering.

@marcphilipp
Copy link
Member

I've submitted #183 today. Would anyone like to review?

@Magnus6803
Copy link

Yes

@Magnus6803
Copy link

That is ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.