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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default reporter for Bazel integration #2399

Merged
merged 6 commits into from Apr 8, 2022

Conversation

lukokr-aarch64
Copy link
Contributor

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.

This allows the user to simply depend on the catch2_main:

cc_test(
    name = "010-TestCase",
    srcs =["examples/010-TestCase.cpp"],
    copts = ["-std=c++14"],
    deps = [":catch2_main"],
)

And Bazel will pick up the test.xml file:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="010-TestCase" errors="0" failures="1" tests="5" hostname="tbd" time="0.000" timestamp="2022-03-23T08:48:13Z">
    <properties>
      <property name="random-seed" value="673971538"/>
    </properties>
    <testcase classname="010-TestCase.global" name="Factorial of 0 is 1 (fail)" time="0.000" status="run">
      <failure message="Factorial(0) == 1" type="REQUIRE">
FAILED:
  REQUIRE( Factorial(0) == 1 )
with expansion:
  0 == 1
at examples/010-TestCase.cpp:11
      </failure>
    </testcase>
    <testcase classname="010-TestCase.global" name="Factorials of 1 and higher are computed (pass)" time="0.000" status="run"/>
    <system-out/>
    <system-err/>
  </testsuite>
</testsuites>

But when running bazelisk test //... --test_output=all Bazel will show the user readable output:

INFO: From Testing //:010-TestCase:
==================== Test output for //:010-TestCase:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
010-TestCase is a Catch2 v3.0.0-preview.4 host application.
Run with -? for options

Randomness seeded to: 3971943524

-------------------------------------------------------------------------------
Factorial of 0 is 1 (fail)
-------------------------------------------------------------------------------
examples/010-TestCase.cpp:10
...............................................................................

examples/010-TestCase.cpp:11: FAILED:
  REQUIRE( Factorial(0) == 1 )
with expansion:
  0 == 1

===============================================================================
test cases: 2 | 1 passed | 1 failed
assertions: 5 | 4 passed | 1 failed

================================================================================

This was originally proposed in #2333 which was reworked with multi-reporter, added a test and fixed the bazel build.

I think from Bazel PoV it is preferable to keep XML_OUTPUT_FILE as a generic env variable that other frameworks recognise based on this TODO: https://github.com/bazelbuild/bazel/blob/1024358d1160a7eafb760300abd4533f160c33f2/tools/test/test-setup.sh#L108-L110

But if that's absolutely not acceptable I could try upstreaming BAZEL_XML_OUTPUT_FILE and change this PR, let me know what you think 馃槃

@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #2399 (7f99ee9) into devel (4b78157) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            devel    #2399   +/-   ##
=======================================
  Coverage   91.11%   91.11%           
=======================================
  Files         155      155           
  Lines        7425     7425           
=======================================
  Hits         6765     6765           
  Misses        660      660           

@lukokr-aarch64
Copy link
Contributor Author

Hi @horenmar, I picked this up from @mattyclarkson and his original PR #2333, I took your advice and used the work you did with multi-reporter. I would appreciate your review if you have a moment.
Cheers!

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.

I only spot-checked the changes to BUILD.bazel, otherwise this seems to be on the right track.

docs/configuration.md Show resolved Hide resolved
src/catch2/catch_user_config.hpp.in Show resolved Hide resolved
src/catch2/catch_config.cpp Outdated Show resolved Hide resolved
src/catch2/catch_config.cpp Outdated Show resolved Hide resolved
src/catch2/catch_config.cpp Outdated Show resolved Hide resolved
tests/CMakeLists.txt Outdated Show resolved Hide resolved
BUILD.bazel Outdated Show resolved Hide resolved
@Vertexwahn
Copy link
Contributor

Vertexwahn commented Apr 1, 2022

Please replace the WORKSPACE file with the following content:

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "bazel_skylib",
    strip_prefix = "bazel-skylib-2a87d4a62af886fb320883aba102255aba87275e",
    urls = [
        "https://github.com/bazelbuild/bazel-skylib/archive/2a87d4a62af886fb320883aba102255aba87275e.tar.gz",
    ],
    sha256 = "d847b08d6702d2779e9eb399b54ff8920fa7521dc45e3e53572d1d8907767de7",
)

load("@bazel_skylib//:workspace.bzl", "bazel_skylib_workspace")

bazel_skylib_workspace()

Currently, bazel-skylib from https://github.com/Vertexwahn/bazel-skylib is used. Luckily, the changes I did to bazel-skylib are now part of the official bazel-skylib main branch

See also here: #2405

@lukokr-aarch64
Copy link
Contributor Author

Thanks for the review, I will address your comments and push a new patchset

BUILD.bazel Outdated Show resolved Hide resolved
lukokr-aarch64 and others added 2 commits April 8, 2022 14:21
When the added Bazel configuration flag is enabled,
a default JUnit reporter will be added if the XML
envrioment variable is defined.
Fix include paths for generated config header.
Enable Bazel config by default when building with
Bazel.
@lukokr-aarch64
Copy link
Contributor Author

Hey thanks @horenmar for fixing the reporter spec, I was wresting with the default constructor on some of the builds.

@horenmar
Copy link
Member

horenmar commented Apr 8, 2022

I am gonna let the Appveyor CI run and merge it when I have time afterwards -- I don't expect it to fail though.

Anyway, two more things

BAZEL_XML_OUTPUT_FILE

I would like to see it upstreamed, because with the prefixed name it doesn't have to be conditionally compiled. As it is, it is squatting on pretty valuable real estate in terms of env vars (squatting on valuable real estate seems to be a pattern with google c++ tho grumble grumble).

  1. Please don't force push every change into single commit next time, it makes reviewing the review fixes hard, because I have to read the full thing again.

@horenmar horenmar merged commit cb551b4 into catchorg:devel Apr 8, 2022
@lukokr-aarch64
Copy link
Contributor Author

Agreed, on both points. Thanks again @horenmar. I'll raise a PR in Bazel regarding the env var.

mattyclarkson added a commit to mattyclarkson/bazel that referenced this pull request May 3, 2022
[Catch2][catch2] has [learnt][pr2399] how to output JUnit to the `XML_OUTPUT_FILE`. However, the upstream developers
[felt][comment] that `XML_OUTPUT_FILE` is too generic to blanket enable JUnit output to the file as there are various
XML reporters for Catch2.

This patch enables prefixing the `XML_OUTPUT_FILE` environment variable with `BAZEL_` so that Catch2 can enable JUnit
output support unconditionally in their build.

[catch2]: https://github.com/catchorg/Catch2
[pr2399]: catchorg/Catch2#2399
[comment]: catchorg/Catch2#2399 (comment)
mattyclarkson added a commit to mattyclarkson/bazel that referenced this pull request May 10, 2022
[Catch2][catch2] has [learnt][pr2399] how to output JUnit to the `XML_OUTPUT_FILE`. However, the upstream developers
[felt][comment] that `XML_OUTPUT_FILE` is too generic to blanket enable JUnit output to the file as there are various
XML reporters for Catch2.

This patch enables exporting `BAZEL_TEST` environment variable so that Catch2 can enable JUnit output support
unconditionally in their build when both `BAZEL_TEST` _and_ `XML_OUTPUT_FILE` are available.

[catch2]: https://github.com/catchorg/Catch2
[pr2399]: catchorg/Catch2#2399
[comment]: catchorg/Catch2#2399 (comment)
mattyclarkson added a commit to mattyclarkson/bazel that referenced this pull request May 10, 2022
[Catch2][catch2] has [learnt][pr2399] how to output JUnit to the `XML_OUTPUT_FILE`. However, the upstream developers
[felt][comment] that `XML_OUTPUT_FILE` is too generic to blanket enable JUnit output to the file as there are various
XML reporters for Catch2.

This patch enables exporting `BAZEL_TEST` environment variable so that Catch2 can enable JUnit output support
unconditionally in their build when both `BAZEL_TEST` _and_ `XML_OUTPUT_FILE` are available.

[catch2]: https://github.com/catchorg/Catch2
[pr2399]: catchorg/Catch2#2399
[comment]: catchorg/Catch2#2399 (comment)
bazel-io pushed a commit to bazelbuild/bazel that referenced this pull request May 10, 2022
[Catch2][catch2] has [learnt][pr2399] how to output JUnit to the `XML_OUTPUT_FILE`. However, the upstream developers
[felt][comment] that `XML_OUTPUT_FILE` is too generic to blanket enable JUnit output to the file as there are various
XML reporters for Catch2.

This patch enables exporting `BAZEL_TEST` environment variable so that Catch2 can enable JUnit output support
unconditionally in their build when both `BAZEL_TEST` _and_ `XML_OUTPUT_FILE` are available.

[catch2]: https://github.com/catchorg/Catch2
[pr2399]: catchorg/Catch2#2399
[comment]: catchorg/Catch2#2399 (comment)

Closes #15393.

PiperOrigin-RevId: 447706388
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