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

Fix issue #1514 where the output of failing tests is not redirected to the reporter #1525

Merged
merged 4 commits into from
Mar 2, 2019

Conversation

SimonChh
Copy link
Contributor

@SimonChh SimonChh commented Feb 3, 2019

Description

Fixes issue where the output of a failing test is not redirected to the reporter (but only when CATCH_CONFIG_EXPERIMENTAL_REDIRECT is not defined).

The issue is caused by invokeActiveTestCase() throwing an exception when the test fails, so the next lines aren't ran. I fixed the error by transferring this code to the destructor of the RAII-style class RedirectedStreams so that it is ran even when an exception occurs.

GitHub Issues

Closes issue #1514 (the output of failing tests is not redirected to the junit reporter)
#1514

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #1525 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1525      +/-   ##
=========================================
+ Coverage   80.58%   80.6%   +0.02%     
=========================================
  Files         121     121              
  Lines        3383    3386       +3     
=========================================
+ Hits         2726    2729       +3     
  Misses        657     657

@codecov
Copy link

codecov bot commented Feb 3, 2019

Codecov Report

Merging #1525 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
+ Coverage   80.54%   80.55%   +0.02%     
==========================================
  Files         121      121              
  Lines        3386     3389       +3     
==========================================
+ Hits         2727     2730       +3     
  Misses        659      659

@@ -67,6 +67,23 @@ namespace Catch {
GeneratorTracker::~GeneratorTracker() {}
}

class RedirectedStreams {
Copy link
Contributor

Choose a reason for hiding this comment

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

this class semantically belongs to include/internal/catch_stream.h with implementation inside include/internal/catch_stream.cpp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I move the class to catch_output_redirect.h/.cpp instead?

I need to reference the RedirectedStdOut and RedirectedStdErr classes from the header file (unfortunately I cannot forward declare them), and these two classes are defined in catch_output_redirect.h (which already #includes catch_stream.h), so I want to avoid the circular include dependency. Plus the class is not really a stream, more a form of output redirection. I could rename the class to something like RedirectOutputToStrings if it's clearer.

@horenmar
Copy link
Member

horenmar commented Mar 2, 2019

Okay, looks good.

@horenmar horenmar merged commit 7d2451f into catchorg:master Mar 2, 2019
@horenmar
Copy link
Member

horenmar commented Mar 2, 2019

Thanks

@horenmar horenmar added the BugFix label Mar 2, 2019
@SimonChh
Copy link
Contributor Author

SimonChh commented Mar 2, 2019

Thanks

No problem. Thanks to you too Martin for maintaining this library.

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

Successfully merging this pull request may close these issues.

3 participants