Skip to content
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.

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

Add Bazel XML_OUTPUT_FILE processing to main #2333

Closed
wants to merge 1 commit into from

Conversation

mattyclarkson
Copy link

bazel test sets the XML_OUTPUT_FILE environment variable which
specifies where the JUnit output of the test executable should be
written.

Picking up this variable allows catch2 to naturally integrate into
the Bazel testing ecosystem out-of-the-box.

The environment variable processing is hidden behind the
CATCH_CONFIG_MAIN_BAZEL_XML_OUTPUT_FILE define and it only
enabled in the BUILD.bazel.

This allows the Bazel ecosystem to create a cc_test target as
follows:

cc_test(
    name = "some_test",
    deps = ["@catch2//:catch2_main"],
    srcs = globs("**/*_test.cpp"),
)

The output will be written to bazel-testlogs/some_test/test.xml

`bazel test` sets the `XML_OUTPUT_FILE` environment variable which
specifies where the JUnit output of the test executable should be
written.

Picking up this variable allows catch2 to naturally integrate into
the Bazel testing ecosystem out-of-the-box.

The environment variable processing is hidden behind the
`CATCH_CONFIG_MAIN_BAZEL_XML_OUTPUT_FILE` define and it _only_
enabled in the `BUILD.bazel`.

This allows the Bazel ecosystem to create a `cc_test` target as
follows:

```
cc_test(
    name = "some_test",
    deps = ["@catch2//:catch2_main"],
    srcs = globs("**/*_test.cpp"),
)
```

The output will be written to `bazel-testlogs/some_test/test.xml`
@mattyclarkson
Copy link
Author

RFC: this is quite a punt to allow Catch2 to integrate more tightly into the Bazel ecosystem out-of-the-box.

It's a bit overzealous about respecting XML_OUTPUT_FILE, included in the default main and currently modifies the Bazel :catch2_main target.

Happy to create a new Bazel target (:catch2_bazel_main?), move the code whereever and be more defensive.

Also happy for it to be rejected: we can add the patches downstream in the Bazel Central Registry, if needed.

@jesseschalken
Copy link

jesseschalken commented Dec 15, 2021

Does this override Catch2's default console output when run via Bazel? If so it would be nice if there was output when run with --test_output=streamed in addition to writing to $XML_OUTPUT_FILE.

@codecov
Copy link

codecov bot commented Dec 15, 2021

Codecov Report

Merging #2333 (f1c5e9f) into devel (dcf9479) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##            devel    #2333   +/-   ##
=======================================
  Coverage   90.96%   90.96%           
=======================================
  Files         151      151           
  Lines        7231     7232    +1     
=======================================
+ Hits         6577     6578    +1     
  Misses        654      654           

@mattyclarkson
Copy link
Author

Does this override Catch2's default console output

Yes. Changing the reporter to junit from console stops the console output.

Catch2 doesn't support mutliple reporters. There is a proposal in #1712 which would enable both JUnit output and console output.

Copy link
Member

@horenmar horenmar left a comment

Choose a reason for hiding this comment

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

Shame Bazel didn't use properly namespaced env variable for this, it could've been enabled unconditionally. XML_OUTPUT_FILE is waaay too generic for that though.

Also this likely shouldn't be in the default-main, instead it should internally add another reporter (if the user doesn't want this behaviour, they can just not provide the define/env variable).

Oh and it obviously needs tests. We already have a bunch of separately-compiled tests in tests/ExtraTests, so you can look there at how to add new one.

return Catch::Session().run( argc, argv );
Catch::Session session;

#if defined(CATCH_CONFIG_MAIN_BAZEL_JUNIT_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.

That's a terrible define name. How about CATCH_CONFIG_BAZEL_SUPPORT? (Does Bazel have more of these customization env option?)

Also it needs to be documented in docs/configuration.md

Copy link
Author

Choose a reason for hiding this comment

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

Agreed. I just sorta went for something. Happy to spin to CATCH_CONFIG_BAZEL_SUPPORT if we don't spin out another Bazel specific main.

auto bazelOutputFile = std::getenv("XML_OUTPUT_FILE");
if (bazelOutputFile != nullptr) {
session.configData().reporterName = "junit";
session.configData().outputFilename = bazelOutputFile;
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be redone with the multireporter support.

Copy link
Author

Choose a reason for hiding this comment

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

Huh, cool. Will look into that.

@mattyclarkson
Copy link
Author

Shame Bazel didn't use properly namespaced env variable for this

Yep. I can ask upstream in Bazel world to see it could also export BAZEL_XML_OUTPUT_FILE.

instead it should internally add another reporter

Like, a whole new Bazel reporter, i.e. --reporter=bazel that inherits from the JUnit reporter?

Oh and it obviously needs tests

Yep. Was pushing this up with minimal code to see if it got straight up rejected. I can move forward with filling out he edges.

Thanks for the comments!

@horenmar
Copy link
Member

Like, a whole new Bazel reporter, i.e. --reporter=bazel that inherits from the JUnit reporter?

No. I meant that Catch2 should internally build another reporter for Bazel when it detects the env variable, instead of having it be done in the main function.

@mattyclarkson
Copy link
Author

instead of having it be done in the main function

So either we add another to the config when it is created or in the session?

Something like:

  • When config parsed
  • If BAZEL_XML_OUTPUT_FILE present
  • Add JUnit multi-reporter pointing at BAZEL_XML_OUTPUT_FILE

or

  • When session started
  • If BAZEL_XML_OUTPUT_FILE present
  • Add JUnit multi-reporter pointing at BAZEL_XML_OUTPUT_FILE

The former might be nicer because dumping the configuration would show the extra JUnit reporter when BAZEL_XML_OUTPUT_FILE is present.

@horenmar
Copy link
Member

Either of these is fine honestly. I would probably go with inserting the new reporter as part of parsed config (I think the ConfigData instance could already be initialized with the reporter pair, but I haven't checked closely if that would run into some issues later).

@horenmar
Copy link
Member

I recently changed how the default reporter is handled, the Bazel reporter can likely be handled in the same way: fc5552d

@mattyclarkson
Copy link
Author

Closing in favour of #2399

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants