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

Call count for free functions requires manual checkpoint #30

Closed
ArekPiekarz opened this issue Aug 11, 2019 · 4 comments · Fixed by #34
Closed

Call count for free functions requires manual checkpoint #30

ArekPiekarz opened this issue Aug 11, 2019 · 4 comments · Fixed by #34
Labels
bug Something isn't working

Comments

@ArekPiekarz
Copy link

I wrote a simple example to see if I could mock external modules with free functions. I noticed that times(x) is not checked on expectations for free functions unless I manually added checkpoint().

Here's a snippet for mocking std::env::current_dir:

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

use cfg_if::cfg_if;

cfg_if! {
    if #[cfg(test)] {
        use crate::std::mock_env::current_dir;
    } else {
        use ::std::env::current_dir;
    }
}

mod std {
    use mockall::automock;

    #[automock]
    pub mod env {
        #[allow(dead_code)]
        pub fn current_dir() -> std::io::Result<std::path::PathBuf> {
            Ok("dummy-result".into())
        }
    }
}

fn formatCurrentDir() -> String {
    format!("You are in: {:?}", current_dir().unwrap())
}

#[test]
fn test_formatCurrentDir() {
    use crate::std::mock_env::*;
    // "times(2)" should break the test, but doesn't
    expect_current_dir().times(2).returning(|| Ok("fake_dir".into()));
    assert_eq!("You are in: \"fake_dir\"", formatCurrentDir());
    // checkpoint();  // uncomment to fail the test
}

Is it an intended behavior, have I missed something or is it a bug? If the first, the requirement of using checkpoint() could be added to the docs of modules - https://docs.rs/mockall/0.3.0/mockall/#modules

@asomers
Copy link
Owner

asomers commented Aug 11, 2019

Nope, that's not intended. It's a bug, but it's really just one symptom of a larger problem: mocked free functions have no context. Mocked methods can use their object for context, but free functions cannot. In addition to the times problem, this means that expectations on free functions can leak from one test to another. For example:

#[test]
fn foo() {
    expect_open().returning(|_| Err(EBADF));
    ...
}

fn bar() {
    expect_open().with(eq("/etc/passwd")).returning(|_| MockFileDescriptor::new())
    ...
    open("/etc/passwd").unwrap();
    ...
}

If the foo test runs first, then bar will fail because the catchall expectation on open is still set. Even if the user adds a global mutex so that the two tests can't run concurrently, the problem would still be present. How best to solve this problem? Here are some ideas:

  1. Create a MockContext object. Under the hood it will grab a global mutex so tests can't conflict with each other. It will also checkpoint all global expectations and clear them on drop. The biggest problem with this approach is that a failed test will poison the Mutex, preventing subsequent tests from running.
  2. Create a MockScenario object that will checkpoint selected expectations only. It would look something like this:
#[test]
fn foo() {
    let s = MockScenario::new();
    let e = expect_open().with(eq("/etc/passwd")).return_const(Err(EPERM));
    s.push(e);
    ...
}
  1. Create a context object that's tied to an individual global function and manages state for that one function. It could include a global Mutex, but would do the minimum amount of damage if poisoned. Every expect_ call would create such a context if it didn't already exist. New expectations could be added to the same context. Like this:
#[test]
fn foo() {
    let c: MockContext = expect_open();
    c.expect().with(eq("/etc/passwd")).return_const(Err(EPERM));
    ...
}

What do you think? I personally don't need to mock free functions very often, so I haven't given it much thought.

@ArekPiekarz
Copy link
Author

I don't know yet which solution would be the best, but I think it should take into account another problem mentioned here - #31. Could any of the solutions presented by you solve it?

@asomers
Copy link
Owner

asomers commented Aug 11, 2019

No need. I think that other issue can be solved completely independently.

@ArekPiekarz
Copy link
Author

It would be great if a final solution was easy to use correctly and hard to misuse.

From the proposals above it seems the 3rd one has the most potential. The MockContext object could be marked with must_use to prevent accidental discarding with just expect_foo();

It looks like the other proposals make creating context guards optional, which increases the discipline required to write and maintain the test code.

asomers added a commit that referenced this issue Aug 22, 2019
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
@asomers asomers mentioned this issue Aug 22, 2019
asomers added a commit that referenced this issue Aug 22, 2019
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
asomers added a commit that referenced this issue Aug 23, 2019
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
asomers added a commit that referenced this issue Aug 24, 2019
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
asomers added a commit that referenced this issue Aug 24, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants