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

Add support for multiple reporters #2183

Merged
merged 9 commits into from
Jan 1, 2022

Conversation

mjerabek
Copy link
Contributor

@mjerabek mjerabek commented Mar 8, 2021

Description

This MR adds support for multiple reporters. CLI syntax: -r xml:output.xml -r console. The -o option is still available and determines the default output file, which is used by listeners and reporters without their own output file specified. Only one reporter may use the default output.

TODO

  • Deal with different stdout capturing strategies of the reporters.
  • Allow specifying --use-colour per reporter.
    • This can wait for its own MR, but the CLI syntax should be decided now. Using : as option delimiter (-r console:out.txt:colour=1) is problematic because of Windows absolute paths (pathologic example: -r console:c:\colour=1).
  • Extend Approval tests to test multiple reporters.
  • Improve documentation.
  • Format the patches using clang-format.

GitHub Issues

Addresses #1712

@mjerabek
Copy link
Contributor Author

mjerabek commented Mar 8, 2021

Approval tests added, but ATM failing because of the unhandled mixed stdout capturing strategies.

@mjerabek
Copy link
Contributor Author

mjerabek commented Mar 8, 2021

Ad stdout capturing:

Suppose we have this code:

TEST_CASE("foo") {
    CHECK(1 == 1);
    std::cout << "Hello\n";
    CHECK(2 == 2);
    std::cout << "World\n";
}

and run it as ./test -s and ./test -s -r console -r xml:out.xml. In case A (no capturing), the messages and reports from the reporter will be interleaved. In case B, we need to capture the output for the XML reporter, but we also need it to be printed to the real stdout.

Which brings me to another thing - if the non-capturing reporter output is directed to a file (./test -s -r console:out.txt), the prints still go to stdout (and not the reporter stream). In other words, non-capturing reporters explicitly don't care about the stdout. In this spirit, the interleaving of stdout/reporter messages to console (or the lack of thereof) should not matter and just printing the captured output after the testcase ends should be enough.

Although it likely will be confusing to users. OTOH having prints intermixed with assertions is bad form anyway.

Proposal: If both capturing and non-capturing reporters are used at the same time, the output is captured and re-printed to stdout/stderr at the end of the testcase. More generally, let us define that the relative ordering of print messages w.r.t. the reporter output is unspecified. Single-reporter cases are not affected, so this doesn't break compatibility.

@horenmar
Copy link
Member

horenmar commented Mar 8, 2021

There is actually multiple things that will need to be handled for multiple reporters in a smarter manner, e.g. what if one of the reporters wants to be informed of all assertion successes, while others do not.

@mjerabek
Copy link
Contributor Author

mjerabek commented Mar 8, 2021

There is actually multiple things that will need to be handled for multiple reporters in a smarter manner, e.g. what if one of the reporters wants to be informed of all assertion successes, while others do not.

That one was easy (or I got it wrong). But there's a catch: AFAICS, the fact if Listeners got all assertions or just the failing ones was determined by the used reporter. Now it's based solely on the Listener's preferences. I consider this a fix, but it might break existing listeners that do not set the preference explicitly (good thing this goes to a major release).

@horenmar
Copy link
Member

One more generic concern: How long are the approval tests now? For me, they are already the longest individual CTest test. This adds a lot more of them.

@horenmar
Copy link
Member

Idea for temporarily workaround for the console colour issue:

Colour support is already kinda broken and only works by accident (and sometimes stops working). There is no sane way to support multiple reporters with Windows colours. Thus, let's disable all colours if there are multiple reporters specified, at least temporarily. Later on the colours can be properly reworked.

@codecov
Copy link

codecov bot commented Dec 19, 2021

Codecov Report

Merging #2183 (b5547f2) into devel (6b55f5d) will increase coverage by 0.15%.
The diff coverage is 96.00%.

@@            Coverage Diff             @@
##            devel    #2183      +/-   ##
==========================================
+ Coverage   90.98%   91.13%   +0.15%     
==========================================
  Files         152      152              
  Lines        7252     7316      +64     
==========================================
+ Hits         6598     6667      +69     
+ Misses        654      649       -5     

@horenmar horenmar force-pushed the dev-multiple-reporters branch 3 times, most recently from 6c2464e to 741914b Compare January 1, 2022 12:58
mjerabek and others added 8 commits January 1, 2022 14:02
This requires a bunch of different changes across the reporter
subsystem.

* We need to handle multiple reporters and their differing
  preferences in `ListeningReporter`, e.g. what to do when
  we mix reporters that capture and don't capture stdout.
* We need to change how the reporter is given output and
  how we parse reporter's output destination from CLI.
* Approval tests need to handle multireporter option
* Call order of listeners/reporters in multireporter
* Adding listeners/reporters properly updates reporter preferences
@horenmar horenmar marked this pull request as ready for review January 1, 2022 13:20
This should provide the same overall stdout/err, but the new
output should feel "faster" for test cases that are entered
and exited multiple times (e.g. due to generators).
@horenmar horenmar merged commit f2f585b into catchorg:devel Jan 1, 2022
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.

2 participants