Skip to content

Enable thread-safe Catch2 assertions in framework tests #857

Description

@knoepfel

Summary

Several test algorithm bodies currently use plain assert(...) instead of Catch2 assertions. These checks disappear under NDEBUG, and a failure aborts the process rather than producing a Catch2 failure with expression diagnostics and test-case attribution.

Catch2 3.13.0, already used by this repository, supports assertions from worker threads when built with CATCH_CONFIG_THREAD_SAFE_ASSERTIONS. This issue should enable that supported Catch2 capability throughout the repository rather than use Phlex resources to serialize assertion-bearing algorithms.

Why Not A catch2_resource

A single-token catch2_resource would serialize every algorithm body that performs an assertion. That changes the concurrency regime under test solely to accommodate the test framework, which can invalidate or weaken tests intended to exercise parallel execution. It also couples Phlex production-facing resource semantics to an implementation detail of the test framework.

Resources remain the appropriate mechanism when the algorithm itself requires a constrained resource, such as ROOT, a database connection pool, a GPU context, or an external simulator. They should not be used merely to protect Catch2 reporting.

A local mutex is not a better alternative. It would have to cover Catch2's complete assertion-reporting path, including successful assertions, and would reimplement a supported Catch2 facility with worse maintenance and diagnostics.

Proposed Change

  1. Configure Catch2 itself with CATCH_CONFIG_THREAD_SAFE_ASSERTIONS, before FetchContent_MakeAvailable(Catch2 ...), so Catch2's compiled implementation and every test translation unit use the same configuration.
  2. Replace Catch2-linked assert calls in framework algorithm bodies with non-fatal CHECK-family assertions.
  3. Remove Catch2-driven serialization and stale comments that claim CHECK cannot run in parallel, while retaining any concurrency limit that the test needs for its own semantics.
  4. Add concise test-contributor guidance: code that can execute on a framework worker thread must use non-fatal CHECK-family assertions, not REQUIRE-family assertions or FAIL.
  5. Exercise the relevant parallel tests repeatedly, including under TSAN where feasible.

Thread-Safety Rules

With CATCH_CONFIG_THREAD_SAFE_ASSERTIONS enabled, Catch2 supports concurrent runtime assertion reporting, including the CHECK family used by the current framework-worker callback sites. This preserves their intended concurrency::unlimited behavior.

The configuration does not make every macro appropriate for a worker thread:

  • Use CHECK, CHECK_FALSE, CHECK_THROWS, CHECK_THROWS_AS, CHECK_THROWS_WITH, CHECK_NOTHROW, and CHECK_THAT in worker-thread code.
  • Do not use REQUIRE, REQUIRE_FALSE, or FAIL in worker-thread code. A failing fatal assertion throws outside Catch2's test-case exception boundary and can terminate the process.
  • Do not invoke SECTION, test-case, generator, or benchmark macros from worker threads.

Current State

The repository currently fetches Catch2 v3.13.0 but does not enable CATCH_CONFIG_THREAD_SAFE_ASSERTIONS. It only configures Catch2 counter handling. As a result, existing CHECK invocations from concurrent TBB callbacks are not protected by Catch2's thread-safe assertion implementation.

The current graph/TBB callback sites use only CHECK; no REQUIRE, REQUIRE_FALSE, or FAIL was found in those paths. Examples that can run concurrently include:

  • test/provider_test.cpp: CHECK in concurrency::unlimited observers.
  • test/unfold_test.cpp: CHECK in unlimited observers.
  • test/multiple_function_registration_test.cpp: CHECK in unlimited transform callbacks.
  • test/filter_test.cpp: CHECK in destructors of graph-owned objects, potentially on framework worker threads.

Some callbacks currently use concurrency::serial solely to avoid Catch2 thread-safety concerns, for example test/output_products_test.cpp. Serial execution prevents simultaneous invocations of that node but does not guarantee execution on Catch2's main test thread, so it is not a complete substitute for Catch2's thread-safe configuration.

Scope

In scope:

  • Enable CATCH_CONFIG_THREAD_SAFE_ASSERTIONS for Catch2 globally and consistently.
  • Convert Catch2-linked framework algorithm assertions from plain assert to CHECK where appropriate, including the existing workaround in test/class_registration_test.cpp.
  • Revisit serialization or comments introduced solely for Catch2 assertion safety, preserving constraints that are independently meaningful to each test.
  • Document the worker-thread assertion rule.
  • Validate parallel assertion use, including sanitizer coverage where practical.

Out of scope:

  • Converting assert calls in plugin, benchmark, and FORM translation units that are not linked against Catch2. Doing so would require a separate decision about their test/reporting architecture.
  • Using the resources API merely as a Catch2 serialization mechanism.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestexecution graphHow the graph is established and executed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions