search

eproxus / meck

A mocking library for Erlang
http://eproxus.github.io/meck
Apache License 2.0
811 stars 231 forks source link

Prevent meck_procs timeouts when stopping #231

Closed aronisstav closed 3 years ago

aronisstav commented 3 years ago

When stopping (e.g. due to meck:unload/1), meck_procs restore the original module, which may be a time consuming operation. If a gen_server:call is used for stopping, there's risk of a timeout that will be translated to a confusing error:{not_mocked}. Using gen_server:stop/1 easily uses an infinity timeout, preventing the issue.

This is fixing a bug where such a time-consuming restoration was causing an end_per_suite operation to fail, confusingly.

aronisstav commented 3 years ago

Hmm... it might be the case that gen_server:stop bypasses sys:suspend, which is what the failed test relies on. A different solution to the same problem is to make 'unload' issue a call with infinite timeout. That will need a custom variant of the "gen_server" function. Shall I try fixing it that way?

eproxus commented 3 years ago

Interesting fix. This might be the cause of a few hard-to-reproduce not_mocked errors previously reported.

Can you reproduce the failed test case on your end?

eproxus commented 3 years ago

Hmm... it might be the case that gen_server:stop bypasses sys:suspend, which is what the failed test relies on. A different solution to the same problem is to make 'unload' issue a call with infinite timeout. That will need a custom variant of the "gen_server" function. Shall I try fixing it that way?

That sounds like a good idea!

aronisstav commented 3 years ago

Updated!

eproxus commented 3 years ago

Seems the exception is triggered somehow. Maybe try locally and remove the try catch to see which exception is thrown there. Can you reproduce the failing test cases?

aronisstav commented 3 years ago

Now tests pass locally. I had accidentally commited some debug-related code.

eproxus commented 3 years ago

Looks good. Can you update the CHANGELOG.md too, and squash everything to one commit?

I'd prefer the commit message to be Increase stop timeout to infinity instead, if you agree.

aronisstav commented 3 years ago

Done!

eproxus commented 3 years ago

@aronisstav Last minor thing: the changelog entry needs to be under ## [Unreleased]. Sorry for not being clearer about that.

aronisstav commented 3 years ago

Sorry, fixed again.

eproxus commented 3 years ago

Thank you! ❤️