Getting CppUTest to open the debugger on test failure

[Steve Hill]

I have a number of devs that have asked whether, in debug mode, a test failure could trigger the JIT debugger. I see that this has recently been raised by someone else on GitHub (https://github.com/cpputest/cpputest/issues/1415)

Having a poke around, it seems that we could achieve this by having NormalTestTerminator::exitCurrentTest() do something similar to UtestShell::crash() if this mode is enabled (using a '-b' - for break - command-line argument, for instance?)

Does this seem like a sensible way to provide this? Is there already a way of doing this that I have missed?

Cheers,

S.

[Bas Vodde]

Hi,

Usually you would want that when mocking… and hence the mocking framework supports a “crashOnFailure” option.

We thought about it also for test failure. But it doesn’t seem to make a lot of sense as the test failure ought to happen in the test and therefore you know the call stack and the error message ought to give you enough information for thinking about why the test fails. So, we decided to not do this for the main testing framework.

I’d be curious about why they would want to do this? Usually when a test fails, starting the debugger shouldn’t be the first reaction….

Bas

--
You received this message because you are subscribed to the Google Groups "cpputest" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cpputest+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/bb29c28d-0c22-49ac-ad0b-1b72dd277578n%40googlegroups.com.

[Steve Hill]

Hi Bas,

Thanks for your reply. FYI, we do not use CppUMock widely at the moment - we tend to use FFF fakes and validate later.

The use case is that we have a lot of historic code where we have retrofitted unit-tests and, in some cases, we have a common function that most tests call to check the unit state is still consistent. When this fails, it is clear what the detail of the failure is but the dev will, almost invariably, need to dig around in the debugger to understand the broader nature of the issue.

Currently, they have to re-run the test-suite (outside of our build system) in the debugger, putting a breakpoint in the test; when that breaks, put a breakpoint in the common validation function; if that is in a loop, keep running until you hit the check that fails or define a conditional breakpoint. They could put a breakpoint in NormalTestTerminator::exitCurrentTest() but I would rather that developers didn't need to rummage around inside the source of CppUTest.

What I would like to provide is for the build system (if in debug mode or optionally in some other way) to run the test suite with the -b argument. This means that, when the test fails, NormalTestTerminator::exitCurrentTest() calls abort() and the JIT debugger kicks in. I've confirmed that calling abort() at that point does do the right thing. We could always make the additional option conditional at compile time, so you have to explicitly enable it in the build of CppUTest but, if the command-line parameter defaults to off anyway, I don't see it as being a big problem.

I'm happy to have a go at making the change, if you are happy with the proposal...

Thanks,

S.

[Bas Vodde]

Hi Steve,

I’m hesitant on this.

I understand your situation… though it seems you need a way out of that rather than an additional option in CppUTest. It is a poor unit test practice to bury the asserts in helper functions as it makes the tests hard to maintain. Debugging poor tests is not really a way out of this. A much better action would be to copy the helper in the test itself and then refactor it to be clearer (or use custom asserts).

Usually we avoid adding features to the framework that promote poor development practices. In the past we thought about this and considered that it would promote poor development practices. Do you think this isn’t the case?

Thanks,

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/e020b4d6-34a4-42f8-83fd-487685dc53e9n%40googlegroups.com.

[Steve Hill]

>> In the past we thought about this and considered that it would promote poor development practices. Do you think this isn’t the case?

That is a good question and I can certainly appreciate your point-of-view. However, I would say that - since this mode would be off by default - it is user-friendly rather than promoting bad practice.

Where I have been working with reasonably well structured and tested modules, I still occasionally get a failure in an existing test where it is not clear what the problem is without resorting to the debugger. Every time this happens, the process of using breakpoints etc. just has too much friction. If I could just rerun the test-suite with -b to pop up the debugger, it would just be so much more frictionless. You can say that the test should have made it clearer what the issue was and you would be right but I still need to debug the problem before I can refactor the test to improve it... so let's make that easier.

On a more pragmatic note, you are also right that our existing code could be (much!) better. However, it is what it is and we can't afford to refactor a couple of million lines of code in one go. Hence, we need a way of working with what we have - both the good and the bad. I have not found that making certain bits of code hard to work with has encouraged refactoring of it; in fact, I get the impression that people just want to get out of there as quickly as possible!

To be clear, we support two modes for our test builds: release and debug. The release build is used most of the time and, for good tests/assertions, is all that is needed. The debug build is used (in the hopefully rare cases) when the cause of the failure is not clear. Hence, the desire to be able to leverage the JIT debugger in this scenario.

Thanks,

S.

[Bas Vodde]

Hi Steve,

Hrmm… I had to give this a lot of thought.

I agree with nearly all you say. I wasn’t suggesting (and would never) to refactor a million LOC at once. However, when an ugly test fails, then it is the right time to fix THAT test rather than to debug it. So, if we would add the functionality to easily drop into the debugger when a test fails then it is less incentives for the already debugger-obsessed developer to actually fix the test.

I guess I’ll accept a properly done PR related to this (as long as the CppUMock CrashOnFailure is also enabled through the command line option). But I do still feel uncomfortable about this and not sure it is the right thing to do. I wish to promote less debugging and more thinking… 

Thanks!

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/c8d7bd45-ab45-403b-ab5e-19a33e8b964en%40googlegroups.com.

[Steve Hill]

Thanks Bas. I can certainly look at knocking up a PR - is there any documentation of the development process etc. for CppUTest and CppUMock?

[Bas Vodde]

Hi,

Nope, just write clean code and good tests :)

Try not to make too large PRs, they will often stay open very long that way.

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/46282c67-d626-44b6-a904-f30da538c90fn%40googlegroups.com.

[Steve Hill]

:)

Looking at CppUMock, it checks whether to CrashOnFailure (calling UT_CRASH, if so) otherwise calling NormalTestTerminator::exitCurrentTest(). If I put UT_CRASH if run with -b in the latter, that covers 3 use-cases:

1. Run without -b and no call to CrashOnFailure: no crashing, move on to next test on failure and report all failures at the end - as now
2. Run without -b with call to CrashOnFailure: mocks crash but assertions move on to next test - as now
3. Run with -b (with or without call to CrashOnFailure): mocks and assertions both crash - additional behaviour

Does that seem like a reasonable way to go?

S.

[Steve Hill]

Oops, just seen that there is already a '-b' argument (to reverse the running order.) Any preference for the argument to use? '-f' (from 'fatal') appears available or we could use a longer argument form like '-crash' or '--crash'?

[Bas Vodde]

Lets do a longer argument :)

-b was for backwards… becuase -r was taken for repeat (rather than reverse) :P

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/0e7239ca-6eef-475e-bd9a-5fe2f0249f30n%40googlegroups.com.

[Bas Vodde]

Hi,

Uhm, no, I guess scenario #2 is not needed anymore.

I guess the crashing behavior needs to be in ExitCurrentTest somehow, so it will eventually move from CppuMock to CppUTest, with the current CrashOnFailure interface enabling it.

Or is there any reason to keep #2 ?

Or the alternative #2 ought to be to be able to enable/disable crashOnFailure so that it can be done on test scope.

Thanks,

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/2fee8a78-2b68-41ca-9c05-787bc2a7e4b3n%40googlegroups.com.

[Steve Hill]

HI Bas,

I think that we should keep #2 for backward compatibility. There may be existing users that have crashOnFail for mocks but don't want to turn on crashing for the regular CppUTest assertions as well. We could certain deprecate crashOnFail at the next major release (either altogether or move it to CppUTest - though I'm not entirely convinced of the use-case for that) but I would rather maintain backward compatibility with this change set and do that as an independent change-set.

I'm having a look at putting something in place (and understanding how everything hangs together) between other things today. With luck I will be able to create a PR later today so perhaps we can discuss your thoughts in the context of that?

Cheers,

S.

[Bas Vodde]

Hi,

Yup.

At the moment, I do not think we need #2. I’m not sure when there is a benefit to having only the mocks crash. I’m not worried about backward compatibility, especially in this case, as I do assume nobody has that on permanently (I certainly hope not) and we can keep the call just extend the behavior to also crashing on normal assertions.

I suggest to start with just adding crashOnFailure and not bother, for the first PR, with the command line options.

Thanks,

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/53f3522c-7a2c-496a-b628-04588aaf0dfbn%40googlegroups.com.

[Steve Hill]

The thing is that it is the command line option that I need - and was requested in issue #1415. I've got this tested and going (and fixed 'make check' which wasn't failing when the tests failed) so will create a pull-request for you to look at. I'm happy to rework in whatever way you feel appropriate... so long as I get my command-line option!

Cheers,

S.

[Bas Vodde]

Hi,

Sure, but the PR is likely to large that way.

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/3725c82c-ff92-4fdc-a2f0-56d60868cec6n%40googlegroups.com.

[Steve Hill]

PR #1420 created. 10 files, +78 -10 lines. I hope that is small enough to allow discussion. I'll put some annotation comments in the PR to make it as easy as possible to navigate.

S.

[Bas Vodde]

Hi,

I have no choice. I do find the PR a bit too large. I’d prefer to separate the functionality change from the command line change.

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/1f338289-f9b0-4e06-9ec3-72334d23f435n%40googlegroups.com.

[Steve Hill]

If you look at the 3 commits, they are structured in that way:

1. Bugfix to 'make check'
2. Command-line change
3. Change in functionality

If you would rather, I can delete this PR and create separate ones? I appreciate the time that you are spending on this so have no desire to make this more time consuming than it needs to be!

S.

[Bas Vodde]

Hiya,

Yes. please make small PRs for changes that are unrelated.

Reason: They are easy to approve and merge.. lumping them together distracts from the purpose of your PR.

Then don’t do the command line code yet, I know how that would look like, but focus on the crashing behavior.

So far, decisions we need to make:

- It feels wrong to add it to NormalTestTerminator as it isn’t a NormalTestTerminator

- That means, we need to be able to change the ‘default’ terminator

- Do we want to do that on UTest level or on TestRegistry level. I’m thinking UTest and leaving the TestRegistry out of it

Smaller PRs make it easier to experiment with it without having the relatalively trivial command line code in the way.

Also, we do need to think about CppUMock at the same time so that the code leaves in a consistent state.

Thank!

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/608a3072-817d-4c82-8943-22f73511da77n%40googlegroups.com.

[Steve Hill]

I agree that it makes sense for this to sit at the UTest level, not in the TestRegistry.

The reason that I put the functionality in the base class is that it needs to be available for all derived classes - that is, both NormalTestTerminator and TestTerminatorWithoutExceptions need to support this mode... and it does want to terminate 'normally' - whatever that means for that class - if the crash function returns. Hence, if we want another TestTerminator class, we will actually need 2 of them and, where the existing classes are used, the code will need to select the correct one to use.

Given that all derived classes need to support this, I think that it makes sense for it to be in the base class - though I think that I should refactor it a bit, if you decide that it is the right way to go.

If you want to add two new TestTerminator classes (CrashingTestTerminator and CrashingTestTerminatorWithoutExceptions), I guess that we would need some sort of factory functions to replace the explicit use of the classes that we have currently, perhaps DefaultTestTerminator() and DefaultTestTerminatorWithoutExceptions()?

I'm happy to make the change either way (or a third way!) so let me have your thoughts and I'll go with whichever way you think is best.

Cheers,

S.

[Steve Hill]

Hi Bas,

I've split out (and refactored a bit) the core change to add a crash-on-fail mode of operation into a new PR. As I said above, I'm happy to rework this to add two more classes and a couple of factory functions. if that is the way you want to go.

Once we have this agreed, we can discuss what you want to do with mock().crashOnFailure() as a second PR?

S.

[Bas Vodde]

Hi,

Why would the crash function not crash?

If there is a good reason, why not create a NormalTestTerminator when the crash function doesn’t crash, inside the TestCrashingTerminator?

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/84c77fe2-0ccc-4629-a10c-67dfbe25f775n%40googlegroups.com.

[Steve Hill]

>> Why would the crash function not crash?

It doesn't crash in the tests for CppUTest!

Let me have a go at reworking my change-set in this way. Would you prefer the factory functions (createDefaultTestTerminator() and  createDefaultTestTerminatorWithoutExceptions()) to be class methods of TestTerminator or straight functions?

Cheers,

S.

[Bas Vodde]

Hi,

Won’t it be the UTest creating them ?

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/5c1529fd-57d8-4d7b-9873-362ed6162d13n%40googlegroups.com.

[Steve Hill]

For NormalTestTerminator, there are only a couple of uses outside of UTest.h/.cpp. TestTerminatorWithoutExceptions is used outside UTest more often. I don't think that it matters though - both factory functions are needed for default parameters on the UTest API so will need global scope. I'll create them as functions - if you want to change them to class methods, that will be trivial to change once everything else is agreed.

S.

[Steve Hill]

Ah, I think that I missed your point - you are suggesting UTestShell::createDefaultTestTerminator() and UTestShell::createDefaultTestTerminatorWithoutExceptions(). Yes, that makes sense; I'll do that.

S.

[Bas Vodde]

Uhm, no :)

But I commented on github to your commits.

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/3cf256bc-e390-4c3f-b574-f7626631f703n%40googlegroups.com.

[Steve Hill]

Hi Bas,

I've updated the PR with the extra tests that you asked for so I hope that this is now ready to be pulled into master. Once that is done, I'll submit the PR to wire it into the command line. In parallel to that I would like to understand what you want to do about mock().crashOnFailure(); do you want to:

1. Remove it altogether - consider it replaced by the run-time, command-line argument
2. Retain it but hook it into the new mechanism (and remove the old) to provide a programmatic means of controlling crash-on-fail mode
3. Replace it with a CppUTest function to provide programmatic control
4. Something else...

Once I know what you want, I'll get the change-set ready to go to PR once the command-line changes have been pulled.

Cheers,

S.

[Bas Vodde]

Hola Steve,

Yeah, I commented on your adding test already. Almost there for the PR.

So far… I think the CppuMock CrashOnFailure interface (the one method) can stay…  but instead of doing anything, we just call the UTestShell one. Then all the behavior can be removed from CppUMock while keeping the interface (the one method, I think)

Thanks!

Bas

To view this discussion on the web visit https://groups.google.com/d/msgid/cpputest/21c0b2ad-a8d5-45b2-b0d4-4692837a96d5n%40googlegroups.com.

[Steve Hill]

Hi Bas,

Thanks for merging my previous PR - I've created a new one for the command-line change.

I've started the work on reworking Mock as agreed. Unfortunately, it looks like it will require the work to support crash-on-fail for C code as a prerequisite (the C++ work was pretty trivial); I assume that you would like this as a separate PR?

S.

[Steve Hill]

Hi Bas,

FYI, I now have all 3 change-sets ready to go:

1. Adding the command-line option: Pull-request already open
2. Add crash-on-fail support to the C testing support
3. Remove the Mock support for its own crash-on-fail and make the crashOnFailure() method provide run-time control of the new mechanism

Since they all build on the previous one(s), I will create the next PR only once the previous one has been accepted. I would really like to close this off so, if you are able to find the time to review the PRs, I would be really grateful.

Cheers,

S.