Fix pickling classes and functions defined in a faulty module

[ogrisel]

• I renamed test_ignoring_whichmodule_exception to test_faulty_module.
• I think this test did not test what it was supposed to test.
• By fixing the test I found a problem in the implementation of save_function which I fixed to make the new version of the test pass.

[codecov-io]

Codecov Report

Merging #136 into master will increase coverage by 0.09%.
The diff coverage is 83.33%.

@@ Coverage Diff @@## master #136 +/- ##
==========================================
+ Coverage 83.76% 83.85% +0.09% 
==========================================
Files 2 2 Lines 536 539 +3 Branches 98 98 ==========================================
+ Hits 449 452 +3 
Misses 64 64 Partials 23 23

Impacted Files	Coverage Δ
cloudpickle/cloudpickle.py	83.76% <83.33%> (+0.09%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7f1b5...bd9ae2c. Read the comment docs.

[ogrisel]

I probably need to add a new test for classes and functions that have .__module__ = None.

[ogrisel]

I do not understand how codecov computes its diff hit percentage. The diff coverage is actually increasing as per https://codecov.io/gh/cloudpipe/cloudpickle/compare/ce7f1b5110ed1b16332d4cabe6c530ef6b8c06e4...c821d972c79f01bd66c0d7eb6bb110b438aa0b47/diff .

[ogrisel]

I cannot use @pytest.mark.parametrize because we derive from unittest.TestCase.

[Peque]

Maybe new tests could be added in a pytest style? (i.e.: as separate functions instead of unittest.TestCase methods) This would result in a bit less boilerplate, less indentation and would allow the use of assert instead of assertEqual().

Eventually we could rewrite all tests with this style if you all like the idea. Should not be too hard. Willing to help with that. 😊

[ogrisel]

We still need to use a base class for the various protocols (but not necessarily inheriting the unittest.TestCase class).

Otherwise one would have to parametrize all the test with the 2 protocols.

Anyway I think it's fine like this for now.

[ogrisel]

@rgbkrk let me merge this one as well. I would like to release 0.5.2 out soon. I will issue other PR for that.

[HyukjinKwon]

Thank you for fixing this.