From f24569a1b4af70352d454ca46eb3be45a0ba51ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Ho=C5=99e=C5=88ovsk=C3=BD?= Date: Sat, 10 Aug 2024 00:28:46 +0200 Subject: [PATCH] Large output redirect refactor This rework changes two important things 1) the output redirect is deactivated while control is given to the reporters. This means that combining reporters that write to stdout with capturing reporters, e.g. `./tests -s -r console -r junit::out=junit.xml`, no longer leads to the capturing reporter seeing all the output from the other reporter captured. Trying this with the `SelfTest` binary would previously lead to JUnit spending **hours** trying to escape all of ConsoleReporter's output and write it to the output file. I actually ended up killing the process after 3 hours, during which the JUnit reporter wrote something like 50 MBs of output to a file. 2) The redirect object's lifetime is tied to the `RunContext`, instead of being constructed for every partial test case run separately. This has no effect on the basic StreamRedirect, but improves the FileRedirect significantly. Previously, running many tests in single process with this redirect (e.g. running `SelfTest -r junit`) would cause later tests to always fail before starting, due to exceeding the limit of temporary files. For the current `SelfTest` binary, the old implementation would lead to **295** test failures from not being able to initiate the redirect. The new implementation completely eliminates them. ---- There is one downside to the new implementation of FileRedirect, specific to Linux. Running the `SelfTest` binary on Linux causes 3-4 tests to have no captured stdout/stderr, even though the tests were supposed to be writing there (there was no output to the actual stdout/stderr either, the output was just completely lost). Since this never happen for smaller test case sets, nor does it reproduce on other platforms, this implementation is still strictly better than the old one, and thus it can get reasonably merged. --- src/catch2/internal/catch_output_redirect.cpp | 411 +++++++++++++----- src/catch2/internal/catch_output_redirect.hpp | 145 +++--- src/catch2/internal/catch_run_context.cpp | 72 +-- src/catch2/internal/catch_run_context.hpp | 4 +- 4 files changed, 402 insertions(+), 230 deletions(-) diff --git a/src/catch2/internal/catch_output_redirect.cpp b/src/catch2/internal/catch_output_redirect.cpp index 02f7c982..245e1376 100644 --- a/src/catch2/internal/catch_output_redirect.cpp +++ b/src/catch2/internal/catch_output_redirect.cpp @@ -5,142 +5,335 @@ // https://www.boost.org/LICENSE_1_0.txt) // SPDX-License-Identifier: BSL-1.0 -#include +#include #include +#include +#include +#include #include #include #include +#include #include -#if defined(CATCH_CONFIG_NEW_CAPTURE) - #if defined(_MSC_VER) - #include //_dup and _dup2 - #define dup _dup - #define dup2 _dup2 - #define fileno _fileno - #else - #include // dup and dup2 - #endif +#if defined( CATCH_CONFIG_NEW_CAPTURE ) +# if defined( _MSC_VER ) +# include //_dup and _dup2 +# define dup _dup +# define dup2 _dup2 +# define fileno _fileno +# else +# include // dup and dup2 +# endif #endif - namespace Catch { - RedirectedStream::RedirectedStream( std::ostream& originalStream, std::ostream& redirectionStream ) - : m_originalStream( originalStream ), - m_redirectionStream( redirectionStream ), - m_prevBuf( m_originalStream.rdbuf() ) - { - m_originalStream.rdbuf( m_redirectionStream.rdbuf() ); - } + namespace { + //! A no-op implementation, used if no reporter wants output + //! redirection. + class NoopRedirect : public OutputRedirect { + void activateImpl() override {} + void deactivateImpl() override {} + std::string getStdout() override { return {}; } + std::string getStderr() override { return {}; } + void clearBuffers() override {} + }; - RedirectedStream::~RedirectedStream() { - m_originalStream.rdbuf( m_prevBuf ); - } + /** + * Redirects specific stream's rdbuf with another's. + * + * Redirection can be stopped and started on-demand, assumes + * that the underlying stream's rdbuf aren't changed by other + * users. + */ + class RedirectedStreamNew { + std::ostream& m_originalStream; + std::ostream& m_redirectionStream; + std::streambuf* m_prevBuf; - RedirectedStdOut::RedirectedStdOut() : m_cout( Catch::cout(), m_rss.get() ) {} - auto RedirectedStdOut::str() const -> std::string { return m_rss.str(); } + public: + RedirectedStreamNew( std::ostream& originalStream, + std::ostream& redirectionStream ): + m_originalStream( originalStream ), + m_redirectionStream( redirectionStream ), + m_prevBuf( m_originalStream.rdbuf() ) {} - RedirectedStdErr::RedirectedStdErr() - : m_cerr( Catch::cerr(), m_rss.get() ), - m_clog( Catch::clog(), m_rss.get() ) - {} - auto RedirectedStdErr::str() const -> std::string { return m_rss.str(); } - - RedirectedStreams::RedirectedStreams(std::string& redirectedCout, std::string& redirectedCerr) - : m_redirectedCout(redirectedCout), - m_redirectedCerr(redirectedCerr) - {} - - RedirectedStreams::~RedirectedStreams() { - m_redirectedCout += m_redirectedStdOut.str(); - m_redirectedCerr += m_redirectedStdErr.str(); - } - -#if defined(CATCH_CONFIG_NEW_CAPTURE) - -#if defined(_MSC_VER) - TempFile::TempFile() { - if (tmpnam_s(m_buffer)) { - CATCH_RUNTIME_ERROR("Could not get a temp filename"); - } - if (fopen_s(&m_file, m_buffer, "w+")) { - char buffer[100]; - if (strerror_s(buffer, errno)) { - CATCH_RUNTIME_ERROR("Could not translate errno to a string"); + void startRedirect() { + m_originalStream.rdbuf( m_redirectionStream.rdbuf() ); } - CATCH_RUNTIME_ERROR("Could not open the temp file: '" << m_buffer << "' because: " << buffer); - } - } -#else - TempFile::TempFile() { - m_file = std::tmpfile(); - if (!m_file) { - CATCH_RUNTIME_ERROR("Could not create a temp file."); - } - } + void stopRedirect() { m_originalStream.rdbuf( m_prevBuf ); } + }; -#endif + /** + * Redirects the `std::cout`, `std::cerr`, `std::clog` streams, + * but does not touch the actual `stdout`/`stderr` file descriptors. + */ + class StreamRedirect : public OutputRedirect { + ReusableStringStream m_redirectedOut, m_redirectedErr; + RedirectedStreamNew m_cout, m_cerr, m_clog; - TempFile::~TempFile() { - // TBD: What to do about errors here? - std::fclose(m_file); - // We manually create the file on Windows only, on Linux - // it will be autodeleted -#if defined(_MSC_VER) - std::remove(m_buffer); -#endif - } + public: + StreamRedirect(): + m_cout( Catch::cout(), m_redirectedOut.get() ), + m_cerr( Catch::cerr(), m_redirectedErr.get() ), + m_clog( Catch::clog(), m_redirectedErr.get() ) {} + void activateImpl() override { + m_cout.startRedirect(); + m_cerr.startRedirect(); + m_clog.startRedirect(); + } + void deactivateImpl() override { + m_cout.stopRedirect(); + m_cerr.stopRedirect(); + m_clog.stopRedirect(); + } + std::string getStdout() override { return m_redirectedOut.str(); } + std::string getStderr() override { return m_redirectedErr.str(); } + void clearBuffers() override { + m_redirectedOut.str( "" ); + m_redirectedErr.str( "" ); + } + }; - FILE* TempFile::getFile() { - return m_file; - } +#if defined( CATCH_CONFIG_NEW_CAPTURE ) - std::string TempFile::getContents() { - std::stringstream sstr; - char buffer[100] = {}; - std::rewind(m_file); - while (std::fgets(buffer, sizeof(buffer), m_file)) { - sstr << buffer; - } - return sstr.str(); - } + // Windows's implementation of std::tmpfile is terrible (it tries + // to create a file inside system folder, thus requiring elevated + // privileges for the binary), so we have to use tmpnam(_s) and + // create the file ourselves there. + class TempFile { + public: + TempFile( TempFile const& ) = delete; + TempFile& operator=( TempFile const& ) = delete; + TempFile( TempFile&& ) = delete; + TempFile& operator=( TempFile&& ) = delete; - OutputRedirect::OutputRedirect(std::string& stdout_dest, std::string& stderr_dest) : - m_originalStdout(dup(1)), - m_originalStderr(dup(2)), - m_stdoutDest(stdout_dest), - m_stderrDest(stderr_dest) { - dup2(fileno(m_stdoutFile.getFile()), 1); - dup2(fileno(m_stderrFile.getFile()), 2); - } +# if defined( _MSC_VER ) + TempFile() { + if ( tmpnam_s( m_buffer ) ) { + CATCH_RUNTIME_ERROR( "Could not get a temp filename" ); + } + if ( fopen_s( &m_file, m_buffer, "wb+" ) ) { + char buffer[100]; + if ( strerror_s( buffer, errno ) ) { + CATCH_RUNTIME_ERROR( + "Could not translate errno to a string" ); + } + CATCH_RUNTIME_ERROR( "Could not open the temp file: '" + << m_buffer + << "' because: " << buffer ); + } + } +# else + TempFile() { + m_file = std::tmpfile(); + if ( !m_file ) { + CATCH_RUNTIME_ERROR( "Could not create a temp file." ); + } + } +# endif - OutputRedirect::~OutputRedirect() { - Catch::cout() << std::flush; - fflush(stdout); - // Since we support overriding these streams, we flush cerr - // even though std::cerr is unbuffered - Catch::cerr() << std::flush; - Catch::clog() << std::flush; - fflush(stderr); + ~TempFile() { + // TBD: What to do about errors here? + std::fclose( m_file ); + // We manually create the file on Windows only, on Linux + // it will be autodeleted +# if defined( _MSC_VER ) + std::remove( m_buffer ); +# endif + } - dup2(m_originalStdout, 1); - dup2(m_originalStderr, 2); + std::FILE* getFile() { return m_file; } + std::string getContents() { + ReusableStringStream sstr; + constexpr long buffer_size = 100; + char buffer[buffer_size + 1] = {}; + long current_pos = ftell( m_file ); + CATCH_ENFORCE( current_pos >= 0, + "ftell failed, errno: " << errno ); + std::rewind( m_file ); + while ( current_pos > 0 ) { + auto read_characters = + std::fread( buffer, + 1, + std::min( buffer_size, current_pos ), + m_file ); + buffer[read_characters] = '\0'; + sstr << buffer; + current_pos -= static_cast( read_characters ); + } + return sstr.str(); + } - m_stdoutDest += m_stdoutFile.getContents(); - m_stderrDest += m_stderrFile.getContents(); - } + void clear() { std::rewind( m_file ); } + + private: + std::FILE* m_file = nullptr; + char m_buffer[L_tmpnam] = { 0 }; + }; + + /** + * Redirects the actual `stdout`/`stderr` file descriptors. + * + * Works by replacing the file descriptors numbered 1 and 2 + * with an open temporary file. + */ + class FileRedirect : public OutputRedirect { + TempFile m_outFile, m_errFile; + int m_originalOut = -1; + int m_originalErr = -1; + + // Flushes cout/cerr/clog streams and stdout/stderr FDs + void flushEverything() { + Catch::cout() << std::flush; + fflush( stdout ); + // Since we support overriding these streams, we flush cerr + // even though std::cerr is unbuffered + Catch::cerr() << std::flush; + Catch::clog() << std::flush; + fflush( stderr ); + } + + public: + FileRedirect(): + m_originalOut( dup( fileno( stdout ) ) ), + m_originalErr( dup( fileno( stderr ) ) ) { + CATCH_ENFORCE( m_originalOut >= 0, "Could not dup stdout" ); + CATCH_ENFORCE( m_originalErr >= 0, "Could not dup stderr" ); + } + + std::string getStdout() override { return m_outFile.getContents(); } + std::string getStderr() override { return m_errFile.getContents(); } + void clearBuffers() override { + m_outFile.clear(); + m_errFile.clear(); + } + + void activateImpl() override { + // We flush before starting redirect, to ensure that we do + // not capture the end of message sent before activation. + flushEverything(); + + int ret; + ret = dup2( fileno( m_outFile.getFile() ), fileno( stdout ) ); + CATCH_ENFORCE( ret >= 0, + "dup2 to stdout has failed, errno: " << errno ); + ret = dup2( fileno( m_errFile.getFile() ), fileno( stderr ) ); + CATCH_ENFORCE( ret >= 0, + "dup2 to stderr has failed, errno: " << errno ); + } + void deactivateImpl() override { + // We flush before ending redirect, to ensure that we + // capture all messages sent while the redirect was active. + flushEverything(); + + int ret; + ret = dup2( m_originalOut, fileno( stdout ) ); + CATCH_ENFORCE( + ret >= 0, + "dup2 of original stdout has failed, errno: " << errno ); + ret = dup2( m_originalErr, fileno( stderr ) ); + CATCH_ENFORCE( + ret >= 0, + "dup2 of original stderr has failed, errno: " << errno ); + } + }; #endif // CATCH_CONFIG_NEW_CAPTURE + } // end namespace + + bool isRedirectAvailable( OutputRedirect::Kind kind ) { + switch ( kind ) { + // These two are always available + case OutputRedirect::None: + case OutputRedirect::Streams: + return true; +#if defined( CATCH_CONFIG_NEW_CAPTURE ) + case OutputRedirect::FileDescriptors: + return true; +#endif + default: + return false; + } + } + + Detail::unique_ptr makeOutputRedirect( bool actual ) { + if ( actual ) { + // TODO: Clean this up later +#if defined( CATCH_CONFIG_NEW_CAPTURE ) + return Detail::make_unique(); +#else + return Detail::make_unique(); +#endif + } else { + return Detail::make_unique(); + } + } + + RedirectGuard scopedActivate( OutputRedirect& redirectImpl ) { + return RedirectGuard( true, redirectImpl ); + } + + RedirectGuard scopedDeactivate( OutputRedirect& redirectImpl ) { + return RedirectGuard( false, redirectImpl ); + } + + OutputRedirect::~OutputRedirect() = default; + + RedirectGuard::RedirectGuard( bool activate, OutputRedirect& redirectImpl ): + m_redirect( &redirectImpl ), + m_activate( activate ), + m_previouslyActive( redirectImpl.isActive() ) { + + // Skip cases where there is no actual state change. + if ( m_activate == m_previouslyActive ) { return; } + + if ( m_activate ) { + m_redirect->activate(); + } else { + m_redirect->deactivate(); + } + } + + RedirectGuard::~RedirectGuard() noexcept( false ) { + if ( m_moved ) { return; } + // Skip cases where there is no actual state change. + if ( m_activate == m_previouslyActive ) { return; } + + if ( m_activate ) { + m_redirect->deactivate(); + } else { + m_redirect->activate(); + } + } + + RedirectGuard::RedirectGuard( RedirectGuard&& rhs ) noexcept: + m_redirect( rhs.m_redirect ), + m_activate( rhs.m_activate ), + m_previouslyActive( rhs.m_previouslyActive ), + m_moved( false ) { + rhs.m_moved = true; + } + + RedirectGuard& RedirectGuard::operator=( RedirectGuard&& rhs ) noexcept { + m_redirect = rhs.m_redirect; + m_activate = rhs.m_activate; + m_previouslyActive = rhs.m_previouslyActive; + m_moved = false; + rhs.m_moved = true; + return *this; + } + } // namespace Catch -#if defined(CATCH_CONFIG_NEW_CAPTURE) - #if defined(_MSC_VER) - #undef dup - #undef dup2 - #undef fileno - #endif +#if defined( CATCH_CONFIG_NEW_CAPTURE ) +# if defined( _MSC_VER ) +# undef dup +# undef dup2 +# undef fileno +# endif #endif diff --git a/src/catch2/internal/catch_output_redirect.hpp b/src/catch2/internal/catch_output_redirect.hpp index dc89223b..51b796ba 100644 --- a/src/catch2/internal/catch_output_redirect.hpp +++ b/src/catch2/internal/catch_output_redirect.hpp @@ -8,110 +8,69 @@ #ifndef CATCH_OUTPUT_REDIRECT_HPP_INCLUDED #define CATCH_OUTPUT_REDIRECT_HPP_INCLUDED -#include -#include -#include +#include -#include -#include +#include #include namespace Catch { - class RedirectedStream { - std::ostream& m_originalStream; - std::ostream& m_redirectionStream; - std::streambuf* m_prevBuf; - - public: - RedirectedStream( std::ostream& originalStream, std::ostream& redirectionStream ); - ~RedirectedStream(); - }; - - class RedirectedStdOut { - ReusableStringStream m_rss; - RedirectedStream m_cout; - public: - RedirectedStdOut(); - auto str() const -> std::string; - }; - - // StdErr has two constituent streams in C++, std::cerr and std::clog - // This means that we need to redirect 2 streams into 1 to keep proper - // order of writes - class RedirectedStdErr { - ReusableStringStream m_rss; - RedirectedStream m_cerr; - RedirectedStream m_clog; - public: - RedirectedStdErr(); - auto str() const -> std::string; - }; - - class RedirectedStreams { - public: - RedirectedStreams(RedirectedStreams const&) = delete; - RedirectedStreams& operator=(RedirectedStreams const&) = delete; - RedirectedStreams(RedirectedStreams&&) = delete; - RedirectedStreams& operator=(RedirectedStreams&&) = delete; - - RedirectedStreams(std::string& redirectedCout, std::string& redirectedCerr); - ~RedirectedStreams(); - private: - std::string& m_redirectedCout; - std::string& m_redirectedCerr; - RedirectedStdOut m_redirectedStdOut; - RedirectedStdErr m_redirectedStdErr; - }; - -#if defined(CATCH_CONFIG_NEW_CAPTURE) - - // Windows's implementation of std::tmpfile is terrible (it tries - // to create a file inside system folder, thus requiring elevated - // privileges for the binary), so we have to use tmpnam(_s) and - // create the file ourselves there. - class TempFile { - public: - TempFile(TempFile const&) = delete; - TempFile& operator=(TempFile const&) = delete; - TempFile(TempFile&&) = delete; - TempFile& operator=(TempFile&&) = delete; - - TempFile(); - ~TempFile(); - - std::FILE* getFile(); - std::string getContents(); - - private: - std::FILE* m_file = nullptr; - #if defined(_MSC_VER) - char m_buffer[L_tmpnam] = { 0 }; - #endif - }; - - class OutputRedirect { + bool m_redirectActive = false; + virtual void activateImpl() = 0; + virtual void deactivateImpl() = 0; public: - OutputRedirect(OutputRedirect const&) = delete; - OutputRedirect& operator=(OutputRedirect const&) = delete; - OutputRedirect(OutputRedirect&&) = delete; - OutputRedirect& operator=(OutputRedirect&&) = delete; + enum Kind { + //! No redirect (noop implementation) + None, + //! Redirect std::cout/std::cerr/std::clog streams internally + Streams, + //! Redirect the stdout/stderr file descriptors into files + FileDescriptors, + }; + virtual ~OutputRedirect(); // = default; - OutputRedirect(std::string& stdout_dest, std::string& stderr_dest); - ~OutputRedirect(); - - private: - int m_originalStdout = -1; - int m_originalStderr = -1; - TempFile m_stdoutFile; - TempFile m_stderrFile; - std::string& m_stdoutDest; - std::string& m_stderrDest; + // TODO: Do we want to check that redirect is not active before retrieving the output? + virtual std::string getStdout() = 0; + virtual std::string getStderr() = 0; + virtual void clearBuffers() = 0; + bool isActive() const { return m_redirectActive; } + void activate() { + assert( !m_redirectActive && "redirect is already active" ); + activateImpl(); + m_redirectActive = true; + } + void deactivate() { + assert( m_redirectActive && "redirect is not active" ); + deactivateImpl(); + m_redirectActive = false; + } }; -#endif + bool isRedirectAvailable( OutputRedirect::Kind kind); + Detail::unique_ptr makeOutputRedirect( bool actual ); + + class RedirectGuard { + OutputRedirect* m_redirect; + bool m_activate; + bool m_previouslyActive; + bool m_moved = false; + + public: + RedirectGuard( bool activate, OutputRedirect& redirectImpl ); + ~RedirectGuard() noexcept( false ); + + RedirectGuard( RedirectGuard const& ) = delete; + RedirectGuard& operator=( RedirectGuard const& ) = delete; + + // C++14 needs move-able guards to return them from functions + RedirectGuard( RedirectGuard&& rhs ) noexcept; + RedirectGuard& operator=( RedirectGuard&& rhs ) noexcept; + }; + + RedirectGuard scopedActivate( OutputRedirect& redirectImpl ); + RedirectGuard scopedDeactivate( OutputRedirect& redirectImpl ); } // end namespace Catch diff --git a/src/catch2/internal/catch_run_context.cpp b/src/catch2/internal/catch_run_context.cpp index 8711352c..00861968 100644 --- a/src/catch2/internal/catch_run_context.cpp +++ b/src/catch2/internal/catch_run_context.cpp @@ -170,6 +170,7 @@ namespace Catch { m_config(_config), m_reporter(CATCH_MOVE(reporter)), m_lastAssertionInfo{ StringRef(), SourceLineInfo("",0), StringRef(), ResultDisposition::Normal }, + m_outputRedirect( makeOutputRedirect( m_reporter->getPreferences().shouldRedirectStdOut ) ), m_includeSuccessfulResults( m_config->includeSuccessfulResults() || m_reporter->getPreferences().shouldReportAllAssertions ) { getCurrentMutableContext().setResultCapture( this ); @@ -236,15 +237,17 @@ namespace Catch { m_reporter->testCasePartialStarting(testInfo, testRuns); const auto beforeRunTotals = m_totals; - std::string oneRunCout, oneRunCerr; - runCurrentTest(oneRunCout, oneRunCerr); + runCurrentTest(); + std::string oneRunCout = m_outputRedirect->getStdout(); + std::string oneRunCerr = m_outputRedirect->getStderr(); + m_outputRedirect->clearBuffers(); redirectedCout += oneRunCout; redirectedCerr += oneRunCerr; const auto singleRunTotals = m_totals.delta(beforeRunTotals); auto statsForOneRun = TestCaseStats(testInfo, singleRunTotals, CATCH_MOVE(oneRunCout), CATCH_MOVE(oneRunCerr), aborting()); - m_reporter->testCasePartialEnded(statsForOneRun, testRuns); + ++testRuns; } while (!m_testCaseTracker->isSuccessfullyCompleted() && !aborting()); @@ -289,7 +292,10 @@ namespace Catch { m_lastAssertionPassed = true; } - m_reporter->assertionEnded(AssertionStats(result, m_messages, m_totals)); + { + auto _ = scopedDeactivate( *m_outputRedirect ); + m_reporter->assertionEnded( AssertionStats( result, m_messages, m_totals ) ); + } if ( result.getResultType() != ResultWas::Warning ) { m_messageScopes.clear(); @@ -306,6 +312,7 @@ namespace Catch { } void RunContext::notifyAssertionStarted( AssertionInfo const& info ) { + auto _ = scopedDeactivate( *m_outputRedirect ); m_reporter->assertionStarting( info ); } @@ -324,7 +331,10 @@ namespace Catch { SectionInfo sectionInfo( sectionLineInfo, static_cast(sectionName) ); m_lastAssertionInfo.lineInfo = sectionInfo.lineInfo; - m_reporter->sectionStarting(sectionInfo); + { + auto _ = scopedDeactivate( *m_outputRedirect ); + m_reporter->sectionStarting( sectionInfo ); + } assertions = m_totals.assertions; @@ -384,7 +394,15 @@ namespace Catch { m_activeSections.pop_back(); } - m_reporter->sectionEnded(SectionStats(CATCH_MOVE(endInfo.sectionInfo), assertions, endInfo.durationInSeconds, missingAssertions)); + { + auto _ = scopedDeactivate( *m_outputRedirect ); + m_reporter->sectionEnded( + SectionStats( CATCH_MOVE( endInfo.sectionInfo ), + assertions, + endInfo.durationInSeconds, + missingAssertions ) ); + } + m_messages.clear(); m_messageScopes.clear(); } @@ -401,15 +419,19 @@ namespace Catch { } void RunContext::benchmarkPreparing( StringRef name ) { - m_reporter->benchmarkPreparing(name); + auto _ = scopedDeactivate( *m_outputRedirect ); + m_reporter->benchmarkPreparing( name ); } void RunContext::benchmarkStarting( BenchmarkInfo const& info ) { + auto _ = scopedDeactivate( *m_outputRedirect ); m_reporter->benchmarkStarting( info ); } void RunContext::benchmarkEnded( BenchmarkStats<> const& stats ) { + auto _ = scopedDeactivate( *m_outputRedirect ); m_reporter->benchmarkEnded( stats ); } void RunContext::benchmarkFailed( StringRef error ) { + auto _ = scopedDeactivate( *m_outputRedirect ); m_reporter->benchmarkFailed( error ); } @@ -440,8 +462,13 @@ namespace Catch { } void RunContext::handleFatalErrorCondition( StringRef message ) { + // TODO: scoped deactivate here? Just give up and do best effort? + // the deactivation can break things further, OTOH so can the + // capture + auto _ = scopedDeactivate( *m_outputRedirect ); + // First notify reporter that bad things happened - m_reporter->fatalErrorEncountered(message); + m_reporter->fatalErrorEncountered( message ); // Don't rebuild the result -- the stringification itself can cause more fatal errors // Instead, fake a result data. @@ -468,7 +495,7 @@ namespace Catch { Counts assertions; assertions.failed = 1; SectionStats testCaseSectionStats(CATCH_MOVE(testCaseSection), assertions, 0, false); - m_reporter->sectionEnded(testCaseSectionStats); + m_reporter->sectionEnded( testCaseSectionStats ); auto const& testInfo = m_activeTestCase->getTestCaseInfo(); @@ -499,7 +526,7 @@ namespace Catch { return m_totals.assertions.failed >= static_cast(m_config->abortAfter()); } - void RunContext::runCurrentTest(std::string & redirectedCout, std::string & redirectedCerr) { + void RunContext::runCurrentTest() { auto const& testCaseInfo = m_activeTestCase->getTestCaseInfo(); SectionInfo testCaseSection(testCaseInfo.lineInfo, testCaseInfo.name); m_reporter->sectionStarting(testCaseSection); @@ -510,18 +537,8 @@ namespace Catch { Timer timer; CATCH_TRY { - if (m_reporter->getPreferences().shouldRedirectStdOut) { -#if !defined(CATCH_CONFIG_EXPERIMENTAL_REDIRECT) - RedirectedStreams redirectedStreams(redirectedCout, redirectedCerr); - - timer.start(); - invokeActiveTestCase(); -#else - OutputRedirect r(redirectedCout, redirectedCerr); - timer.start(); - invokeActiveTestCase(); -#endif - } else { + { + auto _ = scopedActivate( *m_outputRedirect ); timer.start(); invokeActiveTestCase(); } @@ -566,11 +583,12 @@ namespace Catch { void RunContext::handleUnfinishedSections() { // If sections ended prematurely due to an exception we stored their // infos here so we can tear them down outside the unwind process. - for (auto it = m_unfinishedSections.rbegin(), - itEnd = m_unfinishedSections.rend(); - it != itEnd; - ++it) - sectionEnded(CATCH_MOVE(*it)); + for ( auto it = m_unfinishedSections.rbegin(), + itEnd = m_unfinishedSections.rend(); + it != itEnd; + ++it ) { + sectionEnded( CATCH_MOVE( *it ) ); + } m_unfinishedSections.clear(); } diff --git a/src/catch2/internal/catch_run_context.hpp b/src/catch2/internal/catch_run_context.hpp index c749304d..f7f353e2 100644 --- a/src/catch2/internal/catch_run_context.hpp +++ b/src/catch2/internal/catch_run_context.hpp @@ -29,6 +29,7 @@ namespace Catch { class IConfig; class IEventListener; using IEventListenerPtr = Detail::unique_ptr; + class OutputRedirect; /////////////////////////////////////////////////////////////////////////// @@ -115,7 +116,7 @@ namespace Catch { private: - void runCurrentTest( std::string& redirectedCout, std::string& redirectedCerr ); + void runCurrentTest(); void invokeActiveTestCase(); void resetAssertionInfo(); @@ -148,6 +149,7 @@ namespace Catch { std::vector m_unfinishedSections; std::vector m_activeSections; TrackerContext m_trackerContext; + Detail::unique_ptr m_outputRedirect; FatalConditionHandler m_fatalConditionhandler; bool m_lastAssertionPassed = false; bool m_shouldReportUnexpected = true;