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.

Sign up for GitHub

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

Try not to assume a module's file extension is .py #2374

Merged
merged 1 commit into from
Apr 21, 2017
Merged

Try not to assume a module's file extension is .py #2374

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ Justyna Janczyszyn
Kale Kundert
Katarzyna Jachim
Kevin Cox
Kodi B. Arfer
Lee Kamentsky
Lev Maximov
Loic Esteve
Expand Down
5 changes: 5 additions & 0 deletions CHANGELOG.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
* Added documentation related to issue (`#1937`_)
Thanks `@skylarjhdownes`_ for the PR.

* Allow collecting files with any file extension as Python modules (`#2369`_).
Thanks `@Kodiologist`_ for the PR.

*

*
Expand All @@ -23,11 +26,13 @@
.. _@skylarjhdownes: https://github.com/skylarjhdownes
.. _@fabioz: https://github.com/fabioz
.. _@metasyn: https://github.com/metasyn
.. _@Kodiologist: https://github.com/Kodiologist


.. _#1937: https://github.com/pytest-dev/pytest/issues/1937
.. _#2276: https://github.com/pytest-dev/pytest/issues/2276
.. _#2336: https://github.com/pytest-dev/pytest/issues/2336
.. _#2369: https://github.com/pytest-dev/pytest/issues/2369


3.0.7 (2017-03-14)
Expand Down
4 changes: 2 additions & 2 deletions _pytest/python.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import fnmatch
import inspect
import sys
import os
import collections
import math
from itertools import count
Expand Down Expand Up @@ -235,8 +236,7 @@ def getmodpath(self, stopatmodule=True, includemodule=False):
continue
name = node.name
if isinstance(node, Module):
assert name.endswith(".py")
name = name[:-3]
name = os.path.splitext(name)[0]
Copy link
Member

Choose a reason for hiding this comment

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

About a test for this:

I was considering writing a unittest that basically creates a Module instance and calls getmodpath.

But after thinking some more about it I think we need an integration test that demonstrates it is possible to collect a file without a .py extension as a Module object and the entire machinery works, otherwise we might end up introducing a regression later in some other part of the code and breaking this use case without realizing it.

I suggest to create a test that has a conftest.py file which implements the pytest_collect_file hook like this (untested):

def test_collects_different_extensions_as_python(testdir):
    """Ensure we can collect files with different extensions as .py files (#2369)""".
    testdir.makeconftest("""
        def pytest_collect_file(path, parent):
            ext = path.ext
            if ext == ".hy":
                return Module(path, parent)
    """)
    testdir.writefile("""
        # ... 1 test here in Hy ...
    """, ext=".hy")
    result = testdir.runpytest()
    result.stdout.fnmatch_lines('*1 passed*')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right, thanks, a test is in.

if stopatmodule:
if includemodule:
parts.append(name)
Expand Down
28 changes: 28 additions & 0 deletions testing/python/collect.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -841,6 +841,34 @@ def test_makeitem_non_underscore(self, testdir, monkeypatch):
l = modcol.collect()
assert '_hello' not in l

def test_issue2369_collect_module_fileext(self, testdir):
"""Ensure we can collect files with weird file extensions as Python
modules (#2369)"""
# We'll implement a little finder and loader to import files containing
# Python source code whose file extension is ".narf".
testdir.makeconftest("""
import sys, os, imp
from _pytest.python import Module

class Loader:
Copy link
Member

Choose a reason for hiding this comment

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

Clever solution. 😁

I notice though that you are using sys.meta_path.append, and because we run this test in the same process as the main process running pytest, we are leaving that finder there for the rest of pytest's test run.

The simplest solution I can find now is to just use testdir.runpytest_subprocess() to run that test in a separated process to ensure isolation. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me. Done.

Copy link
Member

Choose a reason for hiding this comment

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

for a different protection scheme you can use pytest_configure and pytest_unconfigure to push/pop the extra finder

Copy link
Member

Choose a reason for hiding this comment

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

Good point. Do you think this should be changed, or leaving that as it is is fine?

I'm OK either way TBH.

Copy link
Member

Choose a reason for hiding this comment

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

both ways work, i just wanted to document here

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough! 👍

Would you like to merge this then?

def load_module(self, name):
return imp.load_source(name, name + ".narf")
class Finder:
def find_module(self, name, path=None):
if os.path.exists(name + ".narf"):
return Loader()
sys.meta_path.append(Finder())

def pytest_collect_file(path, parent):
if path.ext == ".narf":
return Module(path, parent)""")
testdir.makefile(".narf", """
def test_something():
assert 1 + 1 == 2""")
# Use runpytest_subprocess, since we're futzing with sys.meta_path.
result = testdir.runpytest_subprocess()
result.stdout.fnmatch_lines('*1 passed*')

def test_setup_only_available_in_subdir(testdir):
sub1 = testdir.mkpydir("sub1")
sub2 = testdir.mkpydir("sub2")
Expand Down