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: Delay check whether we are running under pytest until run time #6540

Merged
merged 2 commits into from
Mar 3, 2018

Conversation

jitseniesen
Copy link
Member

Currently, the test whether the environment variable SPYDER_PYTEST is set (which indicates whether we are running under pytest) is done at import time. However, if the tests are run using the unittest plugin, this import happens before the environment variable is actually set. Thus, this PR delays the test until run time.

With this PR and spyder-ide/spyder-unittest#114, I can run the Spyder test suite successfully within Spyder. 🎉

@pep8speaks
Copy link

pep8speaks commented Feb 27, 2018

Hello @jitseniesen! Thanks for updating the PR.

Line 126:1: E402 module level import not at top of file

Comment last updated on February 28, 2018 at 09:58 Hours UTC

@jitseniesen
Copy link
Member Author

The important change is the one at the top of spyder/config/base.py. The other changes are just renames.

I checked the time taken to evaluate running_under_pytest(), because I was afraid that it might be a bottleneck, but it's only one or two microsecond on my laptop.

The PEP8 complaint is not my fault.

@jitseniesen jitseniesen added this to the v4.0betaX milestone Feb 27, 2018
@ccordoba12 ccordoba12 changed the title Delay check whether we are running under pytest until run time PR: Delay check whether we are running under pytest until run time Feb 27, 2018
@@ -21,7 +21,8 @@

# Local imports
from spyder.app.cli_options import get_options
from spyder.config.base import PYTEST, get_conf_path, running_in_mac_app
from spyder.config.base import (
get_conf_path, running_in_mac_app, running_under_pytest)
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 this to

from spyder.config.base import (get_conf_path, running_in_mac_app,
                                running_under_pytest)

Copy link
Member

Choose a reason for hiding this comment

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

This is just the way we write multi-line imports in most of our codebase :-)

@ccordoba12
Copy link
Member

With this PR and spyder-ide/spyder-unittest#114, I can run the Spyder test suite successfully within Spyder

Really cool! I didn't think this was possible, given the complexity of Spyder's test suite. Thanks a lot for working on this!

I only have one question: any reason to not make this against 3.x? Given that we still have to support that branch, I'd prefer not to deal with two different ways of checking for Pytest in our codebase.

PYTEST = os.environ.get('SPYDER_PYTEST')
def running_under_pytest():
"""
Return True if currently running under py.test.
Copy link
Member

Choose a reason for hiding this comment

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

This is trivially minor, but though while PEP 257 allows it, it seems to be convention that we put the summary one-liner on the first line, next to the '''.

Copy link
Member Author

Choose a reason for hiding this comment

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

Really? I always thought it was the other way around ... I had a quick and random look in our code base. To me, it looks like we have far too few multi-line docstrings and that there is no clear preference for either style. We may need an executive decision here.

Copy link
Member

@goanpeca goanpeca Feb 28, 2018

Choose a reason for hiding this comment

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

Hmmm actually I like either

def func():
    """Summary."""

or

def func():
    """
    Summary.

    Something extra.
    """

I don't like this at all :-p

def func():
    """Summary.

    Something extra.
    """

Copy link
Member

Choose a reason for hiding this comment

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

+1 for

def func():
    """
    Summary.

    Some
    """

I like to write docstrings this way too.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer it as well. My concern was only with regard to consistency with the rest of Spyder's codebase, which seemed to mostly use the """Summary line''' form, but I'd be happy to switch.

@jitseniesen
Copy link
Member Author

any reason to not make this against 3.x?

Only that it did not seem necessary to me and that I hadn't tested it. But I just tested it now and it works, so I will change the base.

@jitseniesen jitseniesen modified the milestones: v4.0betaX, v3.2.8 Feb 28, 2018
@ccordoba12
Copy link
Member

But I just tested it now and it works, so I will change the base.

Great, thanks!

@ccordoba12 ccordoba12 merged commit 715b099 into spyder-ide:3.x Mar 3, 2018
ccordoba12 added a commit that referenced this pull request Mar 3, 2018
@jitseniesen jitseniesen deleted the pytestcheck branch May 24, 2018 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants