dascandy / hippomocks

GNU Lesser General Public License v2.1
192 stars 65 forks source link

cpp11 branch - issues against reporter #68

Open mrAtari opened 7 years ago

mrAtari commented 7 years ago

This is a collector topic. ( No critic, just my thoughts) ;-)

I had a look at the current implementation and this are my remarks so far:

  1. Provide a proper preprocessor seam, so that defaultreporter can easily be exchanged. I would suggest s.th. like switching the include by a preprocessor keyword. The Reporter injection point in MockRepository constructor is IMHO not enough, because you will have to adapt every single test, or at least every MakeSut() in a suite.
  2. Why do you need TestStarted() and TestFinished(). Isn't this something for a Testrunner framework?
  3. Where is the HM_NO_EXCEPTIONS implementation? No reporting, without exceptions? Is hippo without exceptions a possible use case?
  4. I wonder, if it's feasible to transfer the RAISEEXCEPTION stuff to the testing framework, raising some kind of Testframework::Fail() instead of creating your own exception hierarchy. But I don't know if anybody needs s.th. like: assert_throws(...)
  5. LINUX_TARGET -> duplicate code. Provide a proper seam.
dascandy commented 7 years ago
  1. Yes, is intended. Right now if you have no exceptions, you won't get the default reporter, but it would be nice to have a way to inject the default reporter to be used once.
  2. Since there may be an exception unwinding while a mocking error is reported, it's quite possible that it may not be able to report the error immediately. To give you a hook to report the error eventually, TestFinished exists. Since you then may have a pending error in the state of your reporter, it's useful to know when a test starts, so you can wipe it then. That's what TestStarted is for.
  3. It's the ability to disable exceptions. You can define it on the command ilne, and it may be derived from compiler-specific exception availability. If you disable exceptions, you cannot .Throw() on an expectation (for obvious reasons) and you cannot use the default reporter (as it throws exceptions). You can use HippoMocks without exceptions - but you'll have to figure out your own way to handle any errors then. You can consider SjLj'ing out of the test case in your test framework (At the risk of UB). You could flag the test as failed and tell the test FW to start a new process for the rest of the test cases. You could just return to the test FW and keep this test case on the stack, and just keep running from there. All are valid approaches that allow you to do full unit testing without exceptions. But I'm not implementing those reporters (as they're all highly tied to the test framework) so you'll have to make those yourself if you want that.
  4. It's part of the default reporter only right now, so yes, that's already been done. In part because you then don't need the reporter functions or exceptions themselves (as you can use it without).
  5. LINUX_TARGET? Let me check, that shouldn't be there. The backtrace stuff is a hack in the default reporter to help out a bit, but it's not nice. I'd love to deprecate the default handler, actually, in part because of this, but I think it'll never happen. Just look at it as a hack collection of "probably OK solution" for all platforms.
mrAtari commented 7 years ago

I wrote an alternative reporter in order to check the provided interface for usability. My header can be included and compiles with VS2013 (has several warnings because of the inline, but doesn't matter)

noexceptionreporter.zip

Imho the actual version of the Reporter's interface forces tight coupling to hippomocks and is not easily extendable. It would be more flexible to use some kind of ReportFailure(msg, at) and call them from HippoMocks. This would reduce the reporter implementation to 3 Methods only.

The key question is, where do you want the message texts to be generated, by HippoMocks or by the reporter. Imho it's easier to make some stardadized texts and parse them afrerwards to whatever format is needed, e.g like CPPUNIT output is formatted in jUnit format and can be easily reused by many tools then.

Please give a hint, if the proposal matches your intention, or not. If you like it, I can finish in this direction.

dascandy commented 7 years ago

The idea from the interface is to have the tight coupling. The message texts should be customizable in the generator, specifically to allow for filtering, selection, printing more data or changing the format of printing so as to integrate with an IDE, test environment, test framework, CI system or such.

I intend to have some reporters included specifically for some test frameworks so people can use those for some common use cases - say, using it with GTest or Catch, or with XML or JUnit-compatible output.

For example, in the CallMissing callback you get told which call is missing, so you can use that info to selectively print a subset of the mock repo (such as anything that came before this, other calls on the same mock etc.). In ExpectationExceeded you could inform the user about why the call did not match, which calls it looks like and why those did not match. In NoCallMatches you can inform what calls did not match and why they did not match.

I can see your point about tight coupling; in this case though it's intentional. Is it an idea to split the responsibilities in two - formatting and reporting?

mrAtari commented 7 years ago

According to the Single Responsibility Principle, formatting and reporting should be splitted. By the way, Catch already includes a bunch of reporters. I would simply reuse CATCH_FAIL() in a HippoCatchReporter implementation then.