-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 default reporter for Bazel integration #2399
Conversation
ce8b711
to
b404700
Compare
Codecov Report
@@ Coverage Diff @@
## devel #2399 +/- ##
=======================================
Coverage 91.11% 91.11%
=======================================
Files 155 155
Lines 7425 7425
=======================================
Hits 6765 6765
Misses 660 660 |
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. |
There was a problem hiding this 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.
Please replace the
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 |
Thanks for the review, I will address your comments and push a new patchset |
4b73ef1
to
debcf3e
Compare
e399b1d
to
c885389
Compare
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.
Hey thanks @horenmar for fixing the reporter spec, I was wresting with the default constructor on some of the builds. |
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
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).
|
Agreed, on both points. Thanks again @horenmar. I'll raise a PR in Bazel regarding the env var. |
[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)
[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)
[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)
[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
bazel test sets the
XML_OUTPUT_FILE
environment variable whichspecifies 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
:And Bazel will pick up the
test.xml
file:But when running
bazelisk test //... --test_output=all
Bazel will show the user readable output: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-L110But if that's absolutely not acceptable I could try upstreaming
BAZEL_XML_OUTPUT_FILE
and change this PR, let me know what you think 😄