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

Change Bazel XML support to depend upon BAZEL_TEST #2459

Merged
merged 1 commit into from
Jun 17, 2022

Conversation

BrandonThomasJonesARM
Copy link

@BrandonThomasJonesARM BrandonThomasJonesARM commented Jun 13, 2022

Relating to the original issue #2399 here;

Bazel has now been updated to have a cleaner environment variable for catch2 to know when to output to JUnit XML.
Seen here: bazelbuild/bazel#15393

Instead of relying on CATCH_CONFIG_BAZEL_SUPPORT we can now inspect the environment variable BAZEL_TEST. This is better as it allows for 'native' detection of both JUnit XML and Bazel. Originally not done as the detection mechanism was using too generic of a environment name.

@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #2459 (eed01db) into devel (372b757) will increase coverage by 0.14%.
The diff coverage is 57.14%.

❗ Current head eed01db differs from pull request most recent head ceddb58. Consider uploading reports for the commit ceddb58 to get more accurate results

@@            Coverage Diff             @@
##            devel    #2459      +/-   ##
==========================================
+ Coverage   91.53%   91.67%   +0.14%     
==========================================
  Files         159      159              
  Lines        7512     7435      -77     
==========================================
- Hits         6876     6816      -60     
+ Misses        636      619      -17     

@horenmar
Copy link
Member

Thanks, but the old approach should be kept around for backwards compatibility, until next release that breaks backcompat.

This means that Catch2 should check the environment variable for path to write the JUnit XML report to if either it was compiled with CATCH_CONFIG_BAZEL_SUPPORT, or if BAZEL_TEST is set in the environment. (I am not sure whether the value of the BAZEL_TEST env var should also be checked for being "1" and not e.g. "0", but I would err on the side of yes it should.)

You should then add a deprecation notice to the description of CATCH_CONFIG_BAZEL_SUPPORT (use placeholder similar to the "introduced in" one), and to docs/deprecations.md.

@horenmar horenmar added the Tweak label Jun 13, 2022
@BrandonThomasJonesARM
Copy link
Author

Understood, I think these are fair points. I will make the changes and update the PR. Thanks for taking a look.

docs/configuration.md Outdated Show resolved Hide resolved
@horenmar
Copy link
Member

looks good now, thanks.

@horenmar horenmar merged commit 7e4ec43 into catchorg:devel Jun 17, 2022
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.

2 participants