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

Export BAZEL_TEST=1 for bazel test executables #15393

Closed

Conversation

mattyclarkson
Copy link
Contributor

@mattyclarkson mattyclarkson commented May 3, 2022

Catch2 has learnt how to output JUnit to the XML_OUTPUT_FILE. However, the upstream developers
felt 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.

@sgowroji sgowroji added team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website awaiting-review PR is awaiting review from an assigned reviewer labels May 4, 2022
@sgowroji sgowroji requested a review from meteorcloudy May 4, 2022 04:14
@meteorcloudy
Copy link
Member

It is possible to pass extra env var with --test_env, for example, --test_env=BAZEL=1. Can you please check if this works for you?

@mattyclarkson
Copy link
Contributor Author

Can you please check if this works for you?

Well, yes, that would work.

However, do we expect anyone that uses {{catch2}} (or other test frameworks) to add {{--test_env}} to their {{.bazelrc}}. I would say that would be a UX/DX regression for usage of test frameworks. Unless we can propagate a test environment variable upon the usage of @catch2//:catch2_main, for example.

Bazel does test framework specific stuff in the test runner:

# Tell googletest about Bazel sharding.
if [[ -n "${TEST_TOTAL_SHARDS+x}" ]] && ((TEST_TOTAL_SHARDS != 0)); then
  export GTEST_SHARD_INDEX="${TEST_SHARD_INDEX}"
  export GTEST_TOTAL_SHARDS="${TEST_TOTAL_SHARDS}"
fi
export GTEST_TMP_DIR="${TEST_TMPDIR}"

# TODO(ulfjack): Update Gunit to accept XML_OUTPUT_FILE and drop this env
# variable.
GUNIT_OUTPUT="xml:${XML_OUTPUT_FILE}"

Removing GUNIT_OUTPUT would mean that GTest has to accept a very generic XML_OUTPUT_FILE, so seems like introducing a BAZEL=1 into the test runner would also allow GTest to check for BAZEL/XML_OUTPUT_FILE combo.

@meteorcloudy
Copy link
Member

so seems like introducing a BAZEL=1 into the test runner would also allow GTest to check for BAZEL/XML_OUTPUT_FILE combo.

I see, seems like we should go with this option. But maybe choosing a more explicit name? Like BAZEL_TEST_SETUP?

@mattyclarkson
Copy link
Contributor Author

Sure. Maybe BAZEL_TEST=1 as it's running under bazel test?

I'm not here for bikeshedding though and BAZEL_TEST_SETUP would be fine 😄

Depending on the naming I can respin the patch and PR description to match.

@meteorcloudy
Copy link
Member

BAZEL_TEST sounds more clean, let's go with this. Please update this PR ;)

[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 mattyclarkson changed the title Export BAZEL_XML_OUTPUT_FILE Export BAZEL_TEST=1 for bazel test executables May 10, 2022
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Thank you!

@meteorcloudy meteorcloudy added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 10, 2022
@meteorcloudy
Copy link
Member

@sgowroji Please help import this PR once the tests pass ;)

@bazel-io bazel-io closed this in 830d464 May 10, 2022
@sgowroji sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-OSS Issues for the Bazel OSS team: installation, release processBazel packaging, website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants