nrxus / faux

Struct mocking library for Rust
https://nrxus.github.io/faux/
MIT License
411 stars 14 forks source link

`faux::methods` panicked, « return type should not be Self » #50

Closed irevoire closed 1 year ago

irevoire commented 1 year ago

Hello, I wanted to use get rids of our custom mocker in meilisearch in favour of faux, thus I tried it with some of our simplest modules, but it's constantly panicking I don't understand what's going on. I extracted one of our simpler modules in another repo.

Do you know what I'm doing wrong here? https://github.com/irevoire/faux-bug (you should run cargo test to make the panic appear)

nrxus commented 1 year ago

Hey @irevoire , you didn't do anything wrong, this is a limitation of faux regarding functions that return the mocked struct itself wrapped in another object. In this specific case, the new() function returns Result<UpdateFileStore> and faux doesn't know how to deal with that.

The workaround for now is to separate that one function away:

#[cfg_attr(test, faux::create)]
#[derive(Clone, Debug)]
pub struct UpdateFileStore {
    path: PathBuf,
}

#[cfg_attr(test, faux::methods)]
impl UpdateFileStore {
    // this function assumes the path has already been created.
    // Only used internally hence it being private
    fn inner_new(path: PathBuf) -> UpdateFileStore {
        UpdateFileStore { path }
    }

    /* snip the rest of your methods */
}

// this impl is split out just to get aroud `faux` not being able to
// handle functions that return `Result<Self>`. Note that this impl
// does NOT have `#[faux::methods]`
impl UpdateFileStore {
    pub fn new(path: impl AsRef<Path>) -> Result<UpdateFileStore> {
        let path = path.as_ref().join(UPDATE_FILES_PATH);
        std::fs::create_dir_all(&path)?;
        Ok(UpdateFileStore::inner_new(path))
    }
}

The reason why faux does not know how to handle functions that wrap the mocked struct is a bit convoluted but essentially it comes down to faux not knowing how to translate a Result<RealInstance> to a Result<MaybeRealMaybeFakeInstance>. In the case of Result it's easy I could just call .map but it's really hard to generalize so I haven't tackled that yet. I think in practice having something that returns Result<Self, E> or even Option<Self> is so common that I may add that as a stop-gap for now, I'll have to see how difficult that would be to add 🤔

That all being said that error is far from good so at least I'll look into making the error less bad.

nrxus commented 1 year ago

I call this out in the docs for #[methods] but there is a lot of docs so it is easy to get lost: https://docs.rs/faux/latest/faux/attr.methods.html#returning-mockable-struct. I need to figure out a better way to document all the known gotchas.

irevoire commented 1 year ago

Oh, sorry, I totally missed it; it works flawlessly, thank you! And with your solution in #49 I have almost everything I need :thinking:

irevoire commented 1 year ago

Oh also am I right that you should actually write;

#[cfg(not(test))] // <----------- HERE
impl UpdateFileStore {
    pub fn new(path: impl AsRef<Path>) -> Result<UpdateFileStore> {
      ...
    }
}

because if you don't you can't compile the tests?

nrxus commented 1 year ago

You don't need the #[cfg(not(test))]. It is totally okay for that to exist even for tests, it just can't be inside an impl block marked by #[faux::methods]

nrxus commented 1 year ago

@irevoire I have released a new version that should handle the follow return types out of the box without having to take the methods into an impl outside:

I hope this addresses most of your use cases so you don't have to be doing too many changes to your existing code 😄