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

Adding call-checking when tracing is on. #453

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sjstraw
Copy link

@sjstraw sjstraw commented Dec 22, 2014

Added an alternative tracing class, MockCheckedActualCallTrace, derived
from MockCheckedActualCall. (The class, MockActualCallTrace, is still
present, but its use with the tracing feature is disabled.) Access to a
few attributes of MockCheckedActualCall were given to the new class
through protected methods, to support the feature.

The MockSupport class was modified to use the new trace class in place
of the former one.

Tests in MockSupportTest.cpp were enhanced to cover the tracing feature
more completely.

A small correction to MockNamedValue (completing initialization) was
done; as far as I know this is not related to the tracing feature, but was found
during code analysis and appeared to be generally beneficial.

Added an alternative tracing class, MockCheckedActualCallTrace, derived
from MockCheckedActualCall. (The class, MockActualCallTrace, is still
present, but its use with the tracing feature is disabled.) Access to a
few attributes of MockCheckedActualCall were given to the new class
through protected methods, to support the feature.

The MockSupport class was modified to use the new trace class in place
of the former one.

Tests in MockSupportTest.cpp were enhanced to cover the tracing feature
more completely.

A small correction to MockNamedValue (completing initialization) was
done; afaik this is not related to the tracing feature, but was found
during code analysis and appeared to be generally beneficial.
@basvodde
Copy link
Member

Wow @sjstraw, thanks!

We'll need to work a bit on the code and tests though. As is, I won't be merging, but we can work with it. Will that be good for you?

The first big question I have is:

  • Why would you want call checking when tracing? What is the use case this came up?

Then, some other suggestions for improvement:

  • The code has an awful lot of comments. In general, we prefer not to comment things that are obvious. If it is not obvious, we prefer to make it obvious. If that isn't possible, then add a comment.
  • The test you extended is now very big? It seems you added many tests in one test?
  • Doesn't this class now duplicate a lot with the other tracing functionality? Can we do it so that it doesn't do that?
  • Did you test-drive the code? Might be good to practice TDD on it?

To move forward. You mentioned a bug in MockNamedValue. Could we do that one first? Then we'll move from there with the discussion and with making this smaller.

Will that work for you?

@sjstraw
Copy link
Author

sjstraw commented Dec 23, 2014

Sure.

Tracing gives a chronological look at the actual calls. The error reports from the expected calls describes conditions in terms of, well, expectations. It is often helpful to have a clear view of what-happened-in-what-order to understand where expectations and reality parted ways.

As you mentioned to me in the past, tracing is useful to understand a lesser-understood piece of code, in a black-box sort of way. If I give it stimulus A, what actual calls does it produce? How about stimulus ... B?

Checking becomes useful (possibly - at least in my eyes) when you want to take what you observed in tracing and make sure that the CUT continues to act according to your observations. Though one should avoid writing brittle code, once the tracing is done, there are a set of assumptions that will be used to write software that utilizes that CUT, and you don't want those assumptions to be broken.

So you trace and observe what was called. Once you know what is called, with your growing understanding, you expect certain behavior, which you add to the test. With call checking enabled with tracing, you have the opportunity to build up these expected calls while tracing.

That is the basis of my reason to attempt combining the two.

@sjstraw
Copy link
Author

sjstraw commented Dec 23, 2014

I will reduce the degree of comments. If code is unclear, I will refactor to make it so.

Yes, the extended test is large, and covers many use cases. I will break it up.

TDD was used extensively during the development of the code. I currently don't have a piece of legacy code to test-drive the changes on, other than the tests in MockSupportTest.cpp.

Re: "Doesn't this class now duplicate a lot with the other tracing functionality? Can we do it so that it doesn't do that?" The only way I found to combine tracing and call checking was to derive the tracing class off of MockCheckedActualCall. The intention was that the new class would not duplicate, but would replace MockActualCallTrace. Your question reveals that I didn't yet come up with a way to still allow tracing without checking; that will have to be thought through.

@ryanplusplus
Copy link
Contributor

Scott,

Could you implement tracing by 'wrapping' an existing actual call
implementation? To do this you'd intercept and then forward all calls to
the 'wrapped' object. Upon intercepting the calls, you would trace the
calls. To enable checking, you'd wrap the normal implementation and to
disable checking you'd pass in an implementation that does nothing.

Ryan

On Tue, Dec 23, 2014, 3:54 PM Scott Straw notifications@github.com wrote:

I will reduce the degree of comments. If code is unclear, I will refactor
to make it so.

Yes, the extended test is large, and covers many use cases. I will break
it up.

TDD was used extensively during the development of the code. I currently
don't have a piece of legacy code to test-drive the changes on, other than
the tests in MockSupportTest.cpp.

Re: "Doesn't this class now duplicate a lot with the other tracing
functionality? Can we do it so that it doesn't do that?" The only way I
found to combine tracing and call checking was to derive the tracing class
off of MockCheckedActualCall. The intention was that the new class would
not duplicate, but would replace MockActualCallTrace. Your question
reveals that I didn't yet come up with a way to still allow tracing without
checking; that will have to be thought through.


Reply to this email directly or view it on GitHub
#453 (comment).

@basvodde
Copy link
Member

Hmm, but @sjstraw... the problem is that when you use tracing and checking, when a test fails then the test won't continue anymore and neither will the tracing. So that is why tracing and checking is rarely useful as the first unexpected call would end the test and there is no trace :)

@sjstraw
Copy link
Author

sjstraw commented Dec 24, 2014

Exactly. So with tracing as it is now you can either trace, or you can add expectations. You cannot do both.

By deriving from MockCheckedActualCall, I gained the ability to disable the failing of the test. The "failures" still are sent to the reporter, but the test continues. I believe it was done by overriding failtest().

What you have with tracing as it is now may be sufficient for everyone's needs. There are certainly ways around the issues I encountered.

I ran into a desire to have both tracing and checking together when I was part way through adding expectations for a test, and wanted to determine in what order the next few actual calls were being executed. I don't remember all the specifics now, but it seemed that I ran into difficulties when turning on tracing in my test that already had expectations. My intention with this pull request was to allow the opportunity to turn tracing on and off at will while analyzing code behavior and adding expectations.

If there is enough added value for the users of CppUMock in this, then let us continue. Otherwise, our time is precious and we should use it on other things with greater gain.

On Dec 24, 2014, at 2:42 AM, Bas Vodde notifications@github.com wrote:

Hmm, but @sjstraw... the problem is that when you use tracing and checking, when a test fails then the test won't continue anymore and neither will the tracing. So that is why tracing and checking is rarely useful as the first unexpected call would end the test and there is no trace :)


Reply to this email directly or view it on GitHub.

@basvodde
Copy link
Member

Ah ok, so it doesn't actually do checking but it does the failure reporting ?

Will it still mark the test as failed? But it will continue to run?

Thanks for your patience, I think I'm beginning to understand it :)

@sjstraw
Copy link
Author

sjstraw commented Dec 24, 2014

Yes, you have the right idea. It goes through the mechanics of checking calls and parameters, but if the expectations are not met, they are reported as a "MOCK TRACE" instead of a "MOCK FAILURE", and the call that would cause the test to fail is not done. I believe I ended up grabbing some code from the function that is usually called to exit the test, so I could report the failure without exiting the test.

Will it still mark the test as failed? Thanks for the question! I believe that is the case, but it should be tested. I will add one.

Thanks again for your interest in this.

BTW, I will move the small change to MockNamedValue off on a separate pull request, since it is unrelated.

On Dec 24, 2014, at 10:12 AM, Bas Vodde notifications@github.com wrote:

Ah ok, so it doesn't actually do checking but it does the failure reporting ?

Will it still mark the test as failed? But it will continue to run?

Thanks for your patience, I think I'm beginning to understand it :)


Reply to this email directly or view it on GitHub.

@basvodde
Copy link
Member

Ok, so then the big question is whether the old tracing is still useful, right? Your thought is not... and I guess so too. Can we think of any reason to also keep the old tracing?

Another design comment, already made by @ryanplusplus, it might be better to do a decorator than to subclass from MockCheckedActualCall. Do you think that could also work?

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

Successfully merging this pull request may close these issues.

None yet

3 participants