From 765ac08f0811f3a9d2cd6d3bdaa6cbe457457adc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Thu, 22 Oct 2020 14:20:47 +0200 Subject: [PATCH] Fix potential infinite loops in generators combined with section filter The problem was that under specific circumstances, namely that none of their children progressed, `GeneratorTracker` will not progress. This was changed recently, to allow for code like this, where a `SECTION` follows a `GENERATE` at the same level: ```cpp SECTION("A") {} auto a = GENERATE(1, 2); SECTION("B") {} ``` However, this interacted badly with `SECTION` filters (`-c foo`), as they could deactivate all `SECTION`s below a generator, and thus stop it from progressing forever. This commit makes GeneratorTracker check whether there are any filters active, and if they are, it checks whether its section-children can ever run. Fixes #2025 --- include/internal/catch_run_context.cpp | 54 ++++++++++++++++--- include/internal/catch_test_case_tracker.cpp | 8 +++ include/internal/catch_test_case_tracker.h | 4 ++ projects/CMakeLists.txt | 27 ++++++++++ projects/SelfTest/UsageTests/Tricky.tests.cpp | 25 +++++++++ 5 files changed, 111 insertions(+), 7 deletions(-) diff --git a/include/internal/catch_run_context.cpp b/include/internal/catch_run_context.cpp index c7a9f203..fa1b2f6f 100644 --- a/include/internal/catch_run_context.cpp +++ b/include/internal/catch_run_context.cpp @@ -71,13 +71,53 @@ namespace Catch { // `SECTION`s. // **The check for m_children.empty cannot be removed**. // doing so would break `GENERATE` _not_ followed by `SECTION`s. - const bool should_wait_for_child = - !m_children.empty() && - std::find_if( m_children.begin(), - m_children.end(), - []( TestCaseTracking::ITrackerPtr tracker ) { - return tracker->hasStarted(); - } ) == m_children.end(); + const bool should_wait_for_child = [&]() { + // No children -> nobody to wait for + if ( m_children.empty() ) { + return false; + } + // If at least one child started executing, don't wait + if ( std::find_if( + m_children.begin(), + m_children.end(), + []( TestCaseTracking::ITrackerPtr tracker ) { + return tracker->hasStarted(); + } ) != m_children.end() ) { + return false; + } + + // No children have started. We need to check if they _can_ + // start, and thus we should wait for them, or they cannot + // start (due to filters), and we shouldn't wait for them + auto* parent = m_parent; + // This is safe: there is always at least one section + // tracker in a test case tracking tree + while ( !parent->isSectionTracker() ) { + parent = &( parent->parent() ); + } + assert( parent && + "Missing root (test case) level section" ); + + auto const& parentSection = + static_cast( *parent ); + auto const& filters = parentSection.getFilters(); + // No filters -> no restrictions on running sections + if ( filters.empty() ) { + return true; + } + + for ( auto const& child : m_children ) { + if ( child->isSectionTracker() && + std::find( filters.begin(), + filters.end(), + static_cast( *child ) + .trimmedName() ) != + filters.end() ) { + return true; + } + } + return false; + }(); // This check is a bit tricky, because m_generator->next() // has a side-effect, where it consumes generator's current diff --git a/include/internal/catch_test_case_tracker.cpp b/include/internal/catch_test_case_tracker.cpp index 2541a3d1..09a5f4c0 100644 --- a/include/internal/catch_test_case_tracker.cpp +++ b/include/internal/catch_test_case_tracker.cpp @@ -233,6 +233,14 @@ namespace TestCaseTracking { m_filters.insert( m_filters.end(), filters.begin()+1, filters.end() ); } + std::vector const& SectionTracker::getFilters() const { + return m_filters; + } + + std::string const& SectionTracker::trimmedName() const { + return m_trimmed_name; + } + } // namespace TestCaseTracking using TestCaseTracking::ITracker; diff --git a/include/internal/catch_test_case_tracker.h b/include/internal/catch_test_case_tracker.h index 131e2ea0..ccad18fe 100644 --- a/include/internal/catch_test_case_tracker.h +++ b/include/internal/catch_test_case_tracker.h @@ -163,6 +163,10 @@ namespace TestCaseTracking { void addInitialFilters( std::vector const& filters ); void addNextFilters( std::vector const& filters ); + //! Returns filters active in this tracker + std::vector const& getFilters() const; + //! Returns whitespace-trimmed name of the tracked section + std::string const& trimmedName() const; }; } // namespace TestCaseTracking diff --git a/projects/CMakeLists.txt b/projects/CMakeLists.txt index ece7f0f6..aee85974 100644 --- a/projects/CMakeLists.txt +++ b/projects/CMakeLists.txt @@ -419,6 +419,33 @@ set_tests_properties(FilteredSection-1 PROPERTIES FAIL_REGULAR_EXPRESSION "No te add_test(NAME FilteredSection-2 COMMAND $ \#1394\ nested -c NestedRunSection -c s1) set_tests_properties(FilteredSection-2 PROPERTIES FAIL_REGULAR_EXPRESSION "No tests ran") +add_test( + NAME + FilteredSection::GeneratorsDontCauseInfiniteLoop-1 + COMMAND + $ "#2025: original repro" -c "fov_0" +) +set_tests_properties(FilteredSection::GeneratorsDontCauseInfiniteLoop-1 + PROPERTIES + PASS_REGULAR_EXPRESSION "inside with fov: 0" # This should happen + FAIL_REGULAR_EXPRESSION "inside with fov: 1" # This would mean there was no filtering +) + +# GENERATE between filtered sections (both are selected) +add_test( + NAME + FilteredSection::GeneratorsDontCauseInfiniteLoop-2 + COMMAND + $ "#2025: same-level sections" + -c "A" + -c "B" +) +set_tests_properties(FilteredSection::GeneratorsDontCauseInfiniteLoop-2 + PROPERTIES + PASS_REGULAR_EXPRESSION "All tests passed \\(4 assertions in 1 test case\\)" +) + + # AppVeyor has a Python 2.7 in path, but doesn't have .py files as autorunnable add_test(NAME ApprovalTests COMMAND ${PYTHON_EXECUTABLE} ${CATCH_DIR}/scripts/approvalTests.py $) set_tests_properties(ApprovalTests PROPERTIES FAIL_REGULAR_EXPRESSION "Results differed") diff --git a/projects/SelfTest/UsageTests/Tricky.tests.cpp b/projects/SelfTest/UsageTests/Tricky.tests.cpp index 41d0adf4..59d5699a 100644 --- a/projects/SelfTest/UsageTests/Tricky.tests.cpp +++ b/projects/SelfTest/UsageTests/Tricky.tests.cpp @@ -402,3 +402,28 @@ TEST_CASE("#1514: stderr/stdout is not captured in tests aborted by an exception // FAIL aborts the test by throwing a Catch exception FAIL("1514"); } + + +TEST_CASE( "#2025: -c shouldn't cause infinite loop", "[sections][generators][regression][.approvals]" ) { + SECTION( "Check cursor from buffer offset" ) { + auto bufPos = GENERATE_REF( range( 0, 44 ) ); + WHEN( "Buffer position is " << bufPos ) { REQUIRE( 1 == 1 ); } + } +} + +TEST_CASE("#2025: original repro", "[sections][generators][regression][.approvals]") { + auto fov = GENERATE(true, false); + DYNAMIC_SECTION("fov_" << fov) { + std::cout << "inside with fov: " << fov << '\n'; + } +} + +TEST_CASE("#2025: same-level sections", "[sections][generators][regression][.approvals]") { + SECTION("A") { + SUCCEED(); + } + auto i = GENERATE(1, 2, 3); + SECTION("B") { + REQUIRE(i < 4); + } +}