Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Fix error when attempting to view module contents in CollectionsEditor #6081

Merged
merged 6 commits into from
Dec 29, 2017

Conversation

CAM-Gerlach
Copy link
Member

Fixes #6080 , where Spyder would produce an error instead of viewing module contents, likely related to the recent introduction of cloudpickle.

Also includes a regression test making sure the editor is created without error and is properly readonly.

@@ -265,5 +269,14 @@ def test_edit_mutable_and_immutable_types(qtbot, monkeypatch):
readonly=True)


def test_view_module_in_coledit(qtbot, monkeypatch):
"""Checks that modules don't produce an error when trying to open them in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change Checks to Check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops; fixed the one above but missed that one. Thanks for the catch; corrected now.

@ccordoba12
Copy link
Member

@CAM-Gerlach, could you post a screenshot to see how it looks to view a module in the Variable Explorer now?

@ccordoba12 ccordoba12 added this to the v3.2.6 milestone Dec 29, 2017
@CAM-Gerlach
Copy link
Member Author

Sure, here ya go, though it wasn't really at all my doing. Is it SOP to post screenshots of any GUI visible changes from a PR?

image

@CAM-Gerlach
Copy link
Member Author

CAM-Gerlach commented Dec 29, 2017

Not sure what was up with the one failed Travis CI check. My test passed locally in several trials, and did the same for every commit (all three should be functionally identical), every build on every platform, except for the one Py3.5, PyQt4 build on Travis that failed, due to my test causing an exception to occur. However, I told it to rerun that build and, of course, it passed again this time. Any idea how such a simple test could possibly be randomly unreliable in rare cases like that?

It seems the one time it failed, it was because the exception that it now catches wasn't thrown as it was supposed to be inside the deepcopy function, and so it proceeded until it hit a different exception and then errored out. Here's the build output if that's helpful. Of course, I could catch that exception too but that would be silly, since it should never actually throw it in the first place. Seems the problem might be with deepcopy not always throwing the error consistently? If so, how could something relatively simple like that be inconsistent—where's the entropy coming from?

@ccordoba12
Copy link
Member

Any idea how such a simple test could possibly be randomly unreliable in rare cases like that?

Nop, sorry. Sometimes tests fail for no good reason. So please add the flaky decorator to your test to avoid rerunning our entire suite if it fails.

@CAM-Gerlach
Copy link
Member Author

Did so, but now seeing the same failure on a different build, and it seems it either passes all three flaky tests or fails all three, so it seems something further needs to be done. As it seems the unreliability is in deepcopy itself detecting the module's unpickleability and raising the correct exception, I'll just have it catch the AttributeError it is throwing as well and handle it the same way as the TypeError, which should fix the root problem. That should be safe, right? (Hopefully not famous last words, heh...)

@CAM-Gerlach
Copy link
Member Author

Also, out of curiosity, is there a reason import copy is done inside that function and not at the top of the file like usual?

@ccordoba12
Copy link
Member

Also, out of curiosity, is there a reason import copy is done inside that function and not at the top of the file like usual?

That's Pierre's work, so no idea.

@CAM-Gerlach
Copy link
Member Author

It couldn't have anything to do with the issue, I wouldn't think?

Other than that, is this good to go?

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job CAM, as usual!

@ccordoba12 ccordoba12 merged commit 1441924 into spyder-ide:3.x Dec 29, 2017
ccordoba12 added a commit that referenced this pull request Dec 29, 2017
@CAM-Gerlach CAM-Gerlach deleted the fix-module-editor branch January 10, 2018 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants