FIX check the type of the entries in sys.modules

[pierreglaser]

Some modules such as coverage can inject some non-module objects inside sys.modules, creating unpredictable bugs while trying to infer the module of a function. This currently affects one test inside the PyPy entry of cloudpickle CI. This PR adds an additional type check when manipulating entries of sys.modules during _whichmodule.

[pierreglaser]

Note that if we decide to move along with this change, the try...catch statement inside whichmodule is not tested anymore by our test_faulty_module (because FaultyModule won't pass the isintance check). To cover those specific lines, we would need to create a module-like object including a nested construct (like MyMod.MyClass.my_method, where MyClass.__getattr__ raises an error) that we would try to pickle. It sounds like a very complicated test, but if @ogrisel you think it is worth it can write it.

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@090a3b3). Click here to learn what that means.
The diff coverage is 0%.

@@ Coverage Diff @@## master #326 +/- ##
=========================================
Coverage ? 53.05% =========================================
Files ? 2 Lines ? 852 Branches ? 176 =========================================
Hits ? 452 Misses ? 365 Partials ? 35

Impacted Files	Coverage Δ
cloudpickle/cloudpickle_fast.py	0% <0%> (ø)

---

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 090a3b3...12677e6. Read the comment docs.

[ogrisel]

Note that if we decide to move along with this change, the try...catch statement inside whichmodule is not tested anymore by our test_faulty_module (because FaultyModule won't pass the isintance check).

You mean the try / finally block? No I think we should keep it anyway. It's a good practice to have test not introduce any long lasting side effect. Let's keep the test_faulty_module as it is.

To cover those specific lines, we would need to create a module-like object including a nested construct (like MyMod.MyClass.my_method, where MyClass.getattr raises an error) that we would try to pickle. It sounds like a very complicated test, but if @ogrisel you think it is worth it can write it.

I think that would be useful to have a new deeper non-regression test for this indeed.

[pierreglaser]

You mean the try / finally block?

No, I meant these lines.

[pierreglaser]

test_non_module_object_passing_whichmodule is a faithful reproducer of the original issue I spotted on pypy with coverage: a non-module object is inserted in sys.modules and miraculously passes a whichmodule check, but since it is not a module, cloudpickle errors out when carrying out routine checks on it such as _is_dynamic.

I also revamped test_faulty_module to include the coverage of the lines mentioned here.

[ogrisel]

LGTM, just a few comments on inline comments to better explain the why of those tests.

Also please don't forget to document the change in the changelog.