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

Mock context #34

Merged
merged 4 commits into from
Aug 24, 2019
Merged

Mock context #34

merged 4 commits into from
Aug 24, 2019

Conversation

asomers
Copy link
Owner

@asomers asomers commented Aug 22, 2019

Add context objects for mocking static methods

This PR adds Context objects for static methods and free functions. Expectations are still global, but the context object will clean up any global expectations when it drops. This has a few benefits:

  • Call counts will be verified when the context object drops
  • The context object can be used to checkpoint its method
  • "No matching expectation found" errors will no longer poison the static method's global mutex. Panics in the returning method still will, however.

Fixes #30

CC @ArekPiekarz

@asomers asomers force-pushed the mock_context branch 2 times, most recently from ec1609a to 9e2f1f0 Compare August 23, 2019 00:25
@asomers
Copy link
Owner Author

asomers commented Aug 23, 2019

@ArekPiekarz do you want to review this PR?

@ArekPiekarz
Copy link

Sure, I will have more time tomorrow to do it.

@asomers
Copy link
Owner Author

asomers commented Aug 23, 2019

Ok, I'll wait for you.

This commit adds Context objects for static methods and free functions.
Expectations are still global, but the context object will clean up any
global expectations when it drops.  This has a few benefits:

* Call counts will be verified when the context object drops
* The context object can be used to checkpoint its method
* "No matching expectation found" errors will no longer poison the
  static method's global mutex.  Panics in the returning method still
  will, however.

Fixes #30
This commit changes Context objects for static methods of generic
structs.  It removes the method's generic parameters from the Context
object, so only the struct's generic parameters (if any) remain.
Similarly, Context::expect will now only have the method's generic
parameters.
@ArekPiekarz
Copy link

I tested how it works now and I am happy to say it fulfills my suggestions about being hard to misuse. It certainly is safer than the optional context mechanism in Mocktopus.

Maybe the Context struct could be marked #[must_use] to get a compilation warning instead of a runtime error, unless you want creating a context and not setting any expectations to be a valid use case. Calling mocked functions without any expectations will by default fail at runtime without the need for context.

As for bikeshedding, I see the convention of controlling the mocked functions through ones with a prefix expect_ was changed for some of them to a suffix _context. The potential problems could be firstly people having to remember which convention has to be used for different kinds of functions - prefix or suffix. Secondly, functions are often verbs, so i.e. take_context(), remove_context() could be interpreted as operations on the context. Maybe these examples of alternatives for mocking remove could prevent them, but I leave it up to you to decide:
context_remove
contextof_remove
mocked_remove
guarded_remove

I noticed in some cases the runtime errors could be more descriptive, but I don't know if they are related to this pull request or if a new issue should be created, or if they are fixed on other branch.

When a free function is called without a matching expectation we get an error:
thread 'someTest' panicked at 'No matching expectation found'
The problem is that no function name nor runtime argument values are printed. However the stacktrace is helpful in showing which line exactly was called. Similarly to GoogleMock it could also print a list of what expectations it tried to match but failed.

#![feature(proc_macro_hygiene)]
#![allow(non_snake_case)]

use mockall::automock;
use mockall::predicate::eq;
use cfg_if::cfg_if;

cfg_if! {
    if #[cfg(test)] {
        use mock_SomeModule::freeFunction;
    } else {
        use SomeModule::freeFunction;
    }
}

#[cfg_attr(test, automock)]
#[allow(dead_code)]
mod SomeModule {
    pub fn freeFunction(_arg: i32) {}
}

fn myFunc() {
    freeFunction(7);
}

#[test]
fn someTest() {
    let context = mock_SomeModule::freeFunction_context();
    context.expect().with(eq(8)); // not matched
    myFunc();
}

When there are a few expectations and one is not satisfied, we get a function name, but not which expectation exactly is to blame:
thread 'someTest' panicked at 'freeFunction: Expectation called fewer than 1 times'
The stacktrace showed only the beginning and end of the test, but not exact line with the wrong expectation, so in larger code base it could be harder to pinpoint.

#![feature(proc_macro_hygiene)]
#![allow(non_snake_case)]

use mockall::automock;
use mockall::predicate::eq;
use cfg_if::cfg_if;

cfg_if! {
    if #[cfg(test)] {
        use mock_SomeModule::freeFunction;
    } else {
        use SomeModule::freeFunction;
    }
}

#[cfg_attr(test, automock)]
#[allow(dead_code)]
mod SomeModule {
    pub fn freeFunction(_arg: i32) {}
}

fn myFunc() {
    freeFunction(7);
    freeFunction(8);
}

#[test]
fn someTest() {
    let context = mock_SomeModule::freeFunction_context();
    context.expect().times(1).with(eq(7));
    context.expect().times(1).with(eq(7)); // not matched
    context.expect().times(1).with(eq(8));
    myFunc();
}

@asomers
Copy link
Owner Author

asomers commented Aug 24, 2019

  • Good idea regarding must_use. I'll do that in a follow-up PR, and I'll do it for a few other types and functions, too.
  • The prefix/suffix inconsistency bothered me, too. But ultimately I did things this way because it's closer to correct English grammar. In expect_foo, "expect" is a verb and "foo" a noun, so "expect" should come first". In foo_context, "foo" is an adjective and "context" a noun, so "foo" should come first. I could eliminate the inconsistency only by replacing "context" with some other word. For example, contextualize_foo (awkward and hard to type), expect_foo (makes it hard to tell the difference between a method that needs context and one that doesn't, or create_context_for_foo (too hard to type). Unless you can come up with anything better, I'm going to leave it this way for now.
  • I have some ideas about how to print better error messages for unsatisfied expectations and unexpected function calls. I'll do that in a separate PR.

Thanks for your feedback, and happy hacking!

@asomers asomers merged commit 8e5568d into master Aug 24, 2019
@asomers asomers deleted the mock_context branch August 24, 2019 15:48
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.

Call count for free functions requires manual checkpoint
2 participants