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

enableBazelEnvSupport behavior breaks test executions relying on memory allocation free test execution #2881

Open
hofbi opened this issue Jul 4, 2024 · 2 comments

Comments

@hofbi
Copy link

hofbi commented Jul 4, 2024

Describe the bug

Executing a test with Bazel that requires a memory allocation free test execution breaks due to the memory allocation done here.

Expected behavior

The test framework should not behave differently based on the build system that is used.
Ideally, Bazel would set the xml reporter in case it detects catch.
Since this is beyond the scope of this issue, an alternative solution would be to give the user the option to disable this behavior.
Currently, you can only enable this explicitly or it is automatically enabled if used with Bazel: https://github.com/catchorg/Catch2/blob/4e8d92bf02f7d1c8006a0e7a5ecabd8e62d98502/src/catch2/catch_config.cpp#L24C21-L29

Reproduction steps
Steps to reproduce the bug.

  • Have a function that fails/throws if someone uses new
  • Execture the test with bazel test
  • Boom

Platform information:

  • OS: Ubuntu 20.04
  • Compiler+version: GCC 9.3 / Clang 12
  • Catch version: tested with 3.1.0 and 3.6.0 (but should happen with 3.x)

Additional context
Add any other context about the problem here.

@horenmar
Copy link
Member

I doubt that the allocation on that line is what is causing you issues -> it is very unlikely to be the first allocation in the run. At the very least, CLI parsing should've been making allocations before that to initialize itself.

Is the actual issue that the XML reporter allocates during output?

Either way, if you expect Catch2 not to allocate memory during normal run, you will have a bad time. Running TEST_CASE allocates. Passing SECTION allocates. Printing assertion (whether successful or not) likely allocates...

@hofbi
Copy link
Author

hofbi commented Jul 22, 2024

Yep, we also found more places where it alloctes.

One valueable improvement could be the option to disable the Bazel env detection. Right now, we apply the following patch

diff --git a/src/catch2/catch_config.cpp b/src/catch2/catch_config.cpp
index 4465831d..55117a1b 100644
--- a/src/catch2/catch_config.cpp
+++ b/src/catch2/catch_config.cpp
@@ -15,23 +15,7 @@
 
 namespace {
     bool provideBazelReporterOutput() {
-#ifdef CATCH_CONFIG_BAZEL_SUPPORT
-        return true;
-#else
-
-#    if defined( _MSC_VER )
-        // On Windows getenv throws a warning as there is no input validation,
-        // since the switch is hardcoded, this should not be an issue.
-#        pragma warning( push )
-#        pragma warning( disable : 4996 )
-#    endif
-
-        return std::getenv( "BAZEL_TEST" ) != nullptr;
-
-#    if defined( _MSC_VER )
-#        pragma warning( pop )
-#    endif
-#endif
+        return false;
     }
 }

because it is only possible to explicitly enable the reporter but not to disable it.

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

No branches or pull requests

2 participants