Skip to content

Add DoctestScope #38

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

Closed
wants to merge 1 commit into from
Closed

Add DoctestScope #38

wants to merge 1 commit into from

Conversation

jayvdb
Copy link
Member

@jayvdb jayvdb commented Nov 2, 2015

No description provided.

@jayvdb jayvdb mentioned this pull request Nov 2, 2015
@jayvdb
Copy link
Member Author

jayvdb commented Nov 2, 2015

This should fix https://bugs.launchpad.net/pyflakes/+bug/1512184

@bitglue
Copy link
Member

bitglue commented Nov 3, 2015

Is there a way we can add a test for this?

@jayvdb
Copy link
Member Author

jayvdb commented Nov 4, 2015

I'd like ideas on how to effectively test this better.

Checker runs everything during it's constructor, which means an intrusive test harness is needed to make assertions regarding progress within the run. A lot better tests could be constructed if Checker.__init__ saved more of its state before returning. e.g. putting a scope tree into a private variable. Anyway, Checker.deadScopes provides a reasonable amount of detail to test the new DoctestScope.

More tests will be possible when the tuples in self._deferredFunctions contains node , which can be pulled out of #37 as a separate changeset. self._deferredFunctions then contains pairs of scopes & node which can be verified are appropriate combinations. self._deferredFunctions is also cleared before __init__ returns, but that could be easily worked around with a test harness.

@bitglue
Copy link
Member

bitglue commented Nov 4, 2015

Well most of the tests are based on checking some snippet, then asserting that particular errors happened, or didn't. If an exception were to be raised in the checking of the snippet that would fail the test. Is there not a snippet that will elicit the bug being fixed here?

@jayvdb
Copy link
Member Author

jayvdb commented Nov 4, 2015

The test test_global_module_scope_pollution verifies that the use of global is contained within the doctest scope and does not pollute the module scope. That is what this section of code was trying to do, and that is what these new tests now confirm.

Anyway, I'll add some more tests which prevent this crash and similar crashes from slipping past code reviewers in the future.

@bitglue
Copy link
Member

bitglue commented Nov 5, 2015

It's looking pretty good. You seem to understand the mechanics of the problem better than I do :) The tests make it pretty clear though, which is good.

I guess all I'm looking for is a snippet that reproduces the traceback originally reported in the bug. The test could be as simple as calling self.flakes() on that code and then asserting that there were no errors, or there were errors, or whatever should happen. The important thing implicitly tested is that it didn't raise an exception.

Fix bug in 03ffc76 caused by determining the doctest global scope level
based on whether parsing doctests was enabled.

Also do not parse docstrings within doctests.
@bitglue
Copy link
Member

bitglue commented Nov 12, 2015

Merged as 93aa3c4. Thanks!

@bitglue bitglue closed this Nov 12, 2015
@jayvdb
Copy link
Member Author

jayvdb commented Nov 12, 2015

@bitglue, Why did you alter the commit? I can not see any changes made in your amended commit.
And it was merge-able...?

@bitglue
Copy link
Member

bitglue commented Nov 13, 2015

I only altered the parent of the commit so there isn't a superfluous merge in the git history.

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