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

Sequence: output on failure identifying which calls were missed #241

Closed
WasabiFan opened this issue Dec 29, 2020 · 4 comments · Fixed by #247
Closed

Sequence: output on failure identifying which calls were missed #241

WasabiFan opened this issue Dec 29, 2020 · 4 comments · Fixed by #247

Comments

@WasabiFan
Copy link

Hi!

Today was my first experience with Sequence. I set up a Sequence with five operations, and in the process had a mismatch between my test and the module under test -- the implementation invoked two functions twice, rather than each only once as the test expected. The result was:

thread 'drivers::current_regulator::tle7242_current_regulator::tests::check_connectivity_succeeds' panicked at 'assertion failed: `(left == right)`   
  left: `4`,
 right: `3`: Method sequence violation', C:\Users\kaeli\.cargo\registry\src\git.luolix.top-1ecc6299db9ec823\mockall-0.9.0\src\lib.rs:1530:9
stack backtrace:
<...>

Through traversing the call stack, I found the invocation of a mocked function which caused the panic, and from there with a bit of reasoning I was able to diagnose the root cause. I had a good amount of trouble discerning what went wrong, since it wasn't clear without looking at the implementation what the two values being asserted were, it was hard to tell whether something was being called too many times or too few, and I wasn't sure what had already been called vs. what was skipped.

Ideally, it would be nice to get an error message printed before the panic which told me where we were in the sequence (including which method was just called!), which calls had correctly occurred, and which ones hadn't correctly (or yet) occurred.

Firstly, is there any interest in this? Is there something else I should be seeing or doing here instead?

I'd be up for giving some improvements a shot in the next few days, if that's welcome and warranted. Is there a straightforward way to implement a more thorough readout without too large a change?

Looking at the way that Sequences are implemented, I must admit, it's quite elegant! Unfortunately, there just isn't a lot of context to go off of. I think if we wanted fully detailed panic messages, there would likely need to be a Vec of call metadata or similar stored in the sequence, to allow it to report on context upon validation failure. Probably wrapped in a mutex. Was this intentionally avoided? It looks like there's no heap allocation at all required in the current implementation, which makes me wonder whether there were perf or other goals here.

@asomers
Copy link
Owner

asomers commented Dec 29, 2020

Would it be useful if the panic message said something like Method sequence violation. 5th method called before 3rd.? That would be an easy change.

@WasabiFan
Copy link
Author

Yeah, I think such a message would be appreciated. That being said, it's rather hard to tell which is the 3rd or 5th — e.g., in my case, some of them are added by helper methods. Capturing names of the functions as part of the mock macro and then including those as well may help, and I think that would be not too much extra from an implementation perspective.

Ideally, what would really be nice — and I recognize there are implementation challenges here — would be to print the ordered function names in a list, so I can visually see which were skipped too.

@asomers
Copy link
Owner

asomers commented Jan 24, 2021

After doing #246 , I realize that it wouldn't be too hard to print the offending method's name and arguments when a Sequence violation occurs. I'm going to try that next.

@WasabiFan
Copy link
Author

I think that would definitely be a welcome feature!

asomers added a commit that referenced this issue Jan 24, 2021
Print the method name and argument values during a method sequence
violation.  Argument values require the "nightly" feature, and the
arguments must implement Debug.

Fixes #241
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 a pull request may close this issue.

2 participants