search

lipanski / mockito

HTTP mocking for Rust!
MIT License
695 stars 59 forks source link

1.3.1 breaks tests that worked before #197

Open threema-donat opened 9 months ago

threema-donat commented 9 months ago

Hey there!

I recently updated the dependencies of a crate via cargo audit, which broke all the tests where we used tokio as runtime and mockito for mocking a http server because of starting a runtime from within a runtime.

I concluded that the version 1.3.1 of mockito should actually have been a breaking release.

lipanski commented 9 months ago

@threema-donat can you share some of the breaking tests or some minimal code to reproduce this? did you manage to fix the tests for 1.3.1 and if so, what did you have to do? could this be related to calling the sync methods from within an async runtime (which was never supported - see the async section inside the docs)?

punkeel commented 8 months ago

+1, our async tests broke after updating mockito v1.1.0 -> v1.4.0.

Cannot start a runtime from within a runtime. This happens because a function (like `block_on`) attempted to block the current thread while the thread is being used to drive asynchronous tasks.

I'm glad they broke, but may I nonetheless suggest detecting this (if at all possible) and printing a more useful error message (pointing to the docs)?

lipanski commented 8 months ago

@punkeel I assume the reason was using the sync methods instead of the async ones inside async tests, right?

This is one of the reasons I mention the error in the docs (though I think it's a slightly different version). I could add the other error message to the docs as well. Not sure if there's any other (easy) way to hint users about this.

This is I guess one of the downsides of supporting both a sync and an async interface but I don't intend to change that any time soon.

punkeel commented 8 months ago

Yep, exactly that

punkeel commented 8 months ago

It looks like it's possible (and easy) to tell if a function is called in the context of a Tokio runtime:

tokio::runtime::Handle::try_current()
    .expect_err("mockito error: use async methods when running inside Tokio - .... link to docs");

Would it make sense to detect this scenario and print a custom message guiding users to the docs?

threema-donat commented 8 months ago

Sorry for the late reply; It was also due to this mistake that our tests broke. A custom message would certainly make the update to the latest version easier.

I would have expected the minor version to be increased if this worked before and not anymore in version '1.3.1'. Also, a note could have been added to the version to make the migration easier for people who mistakenly called the sync methods.

Thanks a lot for your work!