Leave module loaded state as it was #228
Conversation
zsoci
force-pushed
the
restore
branch
2 times, most recently
from
b00b227
to
634668b
Compare
|
Do you think you can add tests for this? Ideally one test with a loaded module and one with an unloaded module. |
Do you mean in addition to the cases that already provide full coverage for the added code? |
|
I think those tests are only implicitly loading the module by running cover? Not sure if there's a test that is equivalent to:
Might be wrong though... |
|
Yes you are right, I think no tests performs step 4. I will create one. |
test/meck_tests.erl
Outdated
| ?assertEqual({module, meck_test_module}, code:load_file(meck_test_module)), | ||
| ok = meck:new(meck_test_module), | ||
| ?assertEqual(ok, meck:unload(meck_test_module)), | ||
| ?assertNotEqual(false, code:is_loaded(meck_test_module)), |
There was a problem hiding this comment.
Can you change this to ?assertMatch({file, _}, code:is_loaded(meck_test_module)),?
There was a problem hiding this comment.
Done. I did not find assertMatch around the line so tried to comply to assertEqual style ;)
There was a problem hiding this comment.
Yeah, unfortunately code:is_loaded/1 does not return a boolean 😄 Otherwise ?assert(not code:is_loaded(...)) would have looked beautiful.
| ok. | ||
| case Restore andalso false =:= code:is_loaded(Mod) of | ||
| true -> | ||
| _ = code:load_file(Mod), |
There was a problem hiding this comment.
Can you add this comment before the call to code:load_file/1?
% We make a best effort to reload the module here. Since this runs
% in a terminating process there is nothing we can do to recover if
% the loading fails.
There was a problem hiding this comment.
Thanks for the good reasoning. Done
|
I think this also fixes #214 |
|
Oh, forgot. One more request. Can you add an entry to |
|
Done. |
| @@ -7,6 +7,8 @@ The format is based on [Keep a Changelog], and this project adheres to | |||
|
|
|||
| ## [Unreleased] | |||
|
|
|||
| - Leave module loaded state as it was [\#228](https://github.com/eproxus/meck/pull/228) | |||
There was a problem hiding this comment.
Last minor nitpick. This should be under a ### Added heading. Otherwise looks good!
There was a problem hiding this comment.
Corrected. Thanks for the clarification. IMO it could also be under Fixed #214
There was a problem hiding this comment.
True, but I feel it is more appropriate to label it as new behavior since it changes the behavior of Meck. It also happens to fix an old issue. 🙂
|
Thanks so much! ❤️ |
No description provided.