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

Register a listener(reporter) that always listens #257

Closed
claremacrae opened this issue Jul 26, 2019 · 11 comments
Closed

Register a listener(reporter) that always listens #257

claremacrae opened this issue Jul 26, 2019 · 11 comments

Comments

@claremacrae
Copy link
Contributor

claremacrae commented Jul 26, 2019

Description

Currently, registering a new reporter only works if the program's command-line arguments request the new reporter.

This is great for a reporter, but it doesn't allow for a subscription model.

For ApprovalTests.cpp, we are looking to say REGISTER_LISTENER(MyReporter) - that will always listen, and not interfere with any use of --reporter on the command line.

Steps to reproduce

If you call REGISTER_REPORTER() without the use of --reporter=, your listener is not used.

Extra information

We (@isidore and I) would love to have a 10 minute Skype call with you, if you are interested.

We are doing this as someone has asked us to make ApprovalTests.cpp support doctest: approvals/ApprovalTests.cpp#11

@claremacrae claremacrae changed the title Register a listener(report) that always listens Register a listener(reporter) that always listens Jul 26, 2019
@claremacrae
Copy link
Contributor Author

On reflection, having looked at the Catch2 code more closely, I see that Catch has two separate concepts, Listener and Reporter.

Catch2 listeners are registered with CATCH_REGISTER_LISTENER() - and are always active. That's what we are using with ApprovalTests.cpp's Catch integration.

Catch2 reporters are selectable by the user at run-time, depending on their needs...

If I understand correctly, doctest has combined these two features in to one - hence this question.

@onqtam
Copy link
Member

onqtam commented Aug 5, 2019

I think it should be doable like this:

context.addFilter("reporters", "my-listener");
context.addFilter("reporters", "console");
// and then call
context.applyCommandLine(argc, argv);

when providing a custom main.

But I'm not sure if that way the console reporter will be replaceable - perhaps the API isn't sufficient yet. Thanks for reporting this - I'll think about it some more when I have the time.

@onqtam
Copy link
Member

onqtam commented Aug 11, 2019

I just pushed this in the dev branch - just use REGISTER_LISTENER instead of REGISTER_REPORTER - the behavior will be the same as in Catch2: all listeners will be executed before any reporter, and cannot be filtered through the command line.

I also just noticed your note about a skype call - I contacted your colleague on Linkedin about a google hangouts which we will arrange there I guess :)

@claremacrae
Copy link
Contributor Author

Thanks! I read the differences in the docs, and I understand the changes because of this request...

I'd be happy to make some minor clarifications to the new docs if you'd be interested - what would be the most convenient way for me to do this?

Comments on the commit?
Fork the repo and make a PR on the dev branch?
Anything else?

@claremacrae
Copy link
Contributor Author

Or pair?

@onqtam
Copy link
Member

onqtam commented Aug 11, 2019

Sorry I left the office. Whichever is easiest for you - comments on the review work too :)

@claremacrae
Copy link
Contributor Author

I can confirm that when I build ApprovalTests.cpp against the doctest single header on its dev branch, and change REGISTER_REPORTER to REGISTER_LISTENER, I can then switch to using the following, and can delete our custom main altogether.

#define DOCTEST_CONFIG_IMPLEMENT_WITH_MAIN
#include <doctest.hpp>

This is tremendous - thanks very much indeed.

I know you're busy, but do you have any idea when you might do your next release, please?

It would be lovely to get this included in ApprovalTests.cpp before I head off for CppCon, to plug the doctest integration as part of my talk there... :-)

@claremacrae
Copy link
Contributor Author

I've also confirmed that the existing reporter mechanism works fine when I have registered a Listener as described above.

For example, running our tests with --reporters=console,xml does do invoke our listener, and does also write out the output of the console and xml reporters. :-)

@claremacrae
Copy link
Contributor Author

As far as I'm concerned, this feature is good to go, and the issue could be closed - thank you again for implementing this! /CC @isidore

@onqtam
Copy link
Member

onqtam commented Aug 12, 2019

I'll release an official version hopefully within 1-2 weeks - way before CppCon :)

@claremacrae
Copy link
Contributor Author

That’s ideal - thank you!

@onqtam onqtam closed this as completed in c241a44 Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants