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

Workaround for #3742 #3751

Merged
merged 2 commits into from Aug 2, 2018

Conversation

Projects
None yet
6 participants
@nicoddemus
Copy link
Member

commented Jul 31, 2018

I did not have time to investigate the issue yet, but took a few minutes to produce a failing test for #3742.

At first I'm not sure how to fix the regression, so I would appreciate suggestions here.

cc @jonozzz

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

This patch fixes the test and breaks another test:

diff --git a/src/_pytest/main.py b/src/_pytest/main.py
index 9599fa16..1aee4bfd 100644
--- a/src/_pytest/main.py
+++ b/src/_pytest/main.py
@@ -482,6 +482,7 @@ class Session(nodes.FSCollector):
             self.trace.root.indent -= 1

     def _collect(self, arg):
+        from _pytest.python import Package
         names = self._parsearg(arg)
         argpath = names.pop(0)
         paths = []
@@ -503,7 +504,7 @@ class Session(nodes.FSCollector):
                             root = self._node_cache[pkginit]
                         else:
                             col = root._collectfile(pkginit)
-                            if col:
+                            if col and isinstance(col, Package):
                                 root = col[0]
                                 self._node_cache[root.fspath] = root

With the above patch, all tests pass except for this one:

________________________________ Test_getinitialnodes.test_pkgfile ________________________________
[gw4] win32 -- Python 3.6.3 c:\pytest\.env36\scripts\python.exe

self = <test_collection.Test_getinitialnodes object at 0x00000283A5DB5630>
testdir = <Testdir local('C:\\Users\\Bruno\\AppData\\Local\\Temp\\pytest-of-Bruno\\pytest-535\\popen-gw4\\test_pkgfile0')>

    def test_pkgfile(self, testdir):
        tmpdir = testdir.tmpdir
        subdir = tmpdir.join("subdir")
        x = subdir.ensure("x.py")
        subdir.ensure("__init__.py")
        with subdir.as_cwd():
            config = testdir.parseconfigure(x)
        col = testdir.getnode(config, x)
        assert isinstance(col, pytest.Module)
        assert col.name == "x.py"
>       assert col.parent.parent.parent is None
E       AttributeError: 'NoneType' object has no attribute 'parent'

c:\pytest\testing\test_collection.py:650: AttributeError

But this test has been changed in c02e8d8 as part of the package-scope fixtures support.

For the record the patch above feels like a hack, but I don't have time today to investigate this further. I would appreciate further suggestions here, and if someone finds a proper solution feel free to push to this branch.

I would like to get 3.7.1 out with a fix ASAP given that this broke a lot of plugins out there.

Only call _collectfile on package instances
As discussed in #3751, this feels like a hack, pushing it only so we can
see how it fares on CI and if there are better solutions out there

@nicoddemus nicoddemus changed the title Add example script and failure for #3742 WIP Add example script and failure for #3742 Jul 31, 2018

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Decided to push the patch above so we can see how it fares under CI and gather feedback.

Unless someone comes up with a better solution I'm considering releasing this fix as it is, at least until we have some time to work on a long term solution.

@jonozzz

This comment has been minimized.

Copy link

commented Jul 31, 2018

I've started looking at this issue today. I applied that patch and it does seem to fix it, however it'll skip the init.py files (tested with flake8 only).

It does look like a hack and I'll see if there's a better fix.

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

Thanks for the feedback @jonozzz!

@coveralls

This comment has been minimized.

Copy link

commented Jul 31, 2018

Coverage Status

Coverage increased (+0.02%) to 92.514% when pulling 8c9efd8 on nicoddemus:collect-file-bug into 2534193 on pytest-dev:master.

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

@jonozzz i beleive the deeper issue comes from packags being collected on the same level as their modules and then dancing a dance around the collectors - a package should be collected from the fspath of a folder

its not yet clear how to reintegrate that propperly

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

Also need to check if this fixes #3749

@nicoddemus nicoddemus force-pushed the nicoddemus:collect-file-bug branch from 3617f1e to 8c9efd8 Aug 1, 2018

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

@RonnyPfannschmidt, you marked test_collect_custom_items as xfail, but this test reproduces the problem and this patch fixes it. 😅

@nicoddemus nicoddemus changed the title WIP Add example script and failure for #3742 Add example script and failure for #3742 Aug 1, 2018

@nicoddemus nicoddemus changed the title Add example script and failure for #3742 Workaround for #3742 Aug 1, 2018

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Aug 1, 2018

yikes, my bad

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

I guess we can merge this once CI passes?

We definitely should investigate a better solution though.

@nicoddemus

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2018

@RonnyPfannschmidt want to merge this?

cybergrind added a commit to tipsi/aiozk that referenced this pull request Aug 2, 2018

@cybergrind

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

@nicoddemus seems that it doesn't work for pytest-asyncio.
https://travis-ci.org/tipsi/aiozk/builds/411126128#L2148

@RonnyPfannschmidt

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

@cybergrind this one does not even try to sort out that issue - #3754 does

@@ -503,7 +505,7 @@ def _collect(self, arg):
root = self._node_cache[pkginit]
else:
col = root._collectfile(pkginit)
if col:
if col and isinstance(col, Package):

This comment has been minimized.

Copy link
@RonnyPfannschmidt

RonnyPfannschmidt Aug 2, 2018

Member

this one is part of creating headaches - in future we def need to sort that out

This comment has been minimized.

Copy link
@nicoddemus

nicoddemus Aug 2, 2018

Author Member

Sure, I'm hoping @jonozzz can come up with a good solution to this. 😁

This comment has been minimized.

Copy link
@jonozzz

jonozzz Aug 2, 2018

Totally agree, this needs a major rework, at least the code that went into main.py.

@RonnyPfannschmidt RonnyPfannschmidt merged commit ca04769 into pytest-dev:master Aug 2, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cybergrind

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2018

Oh, just read to message about async_generator and scrolled down to possible patch. Will be more careful next time

@nicoddemus nicoddemus deleted the nicoddemus:collect-file-bug branch Aug 2, 2018

nicoddemus added a commit that referenced this pull request Aug 2, 2018

Add changelog for #3742
This was missing from PR #3751
@drdavella

This comment has been minimized.

Copy link

commented Aug 8, 2018

We were bitten by the bug reported in #3742, but the fix has led to a new issue: astropy/pytest-doctestplus#22.

I haven't done enough investigation to determine if there's a workaround on our end, but I just wanted to report it here in case anyone else is experiencing similar problems as of 3.7.1.

jonozzz pushed a commit to jonozzz/pytest that referenced this pull request Aug 10, 2018

RonnyPfannschmidt added a commit that referenced this pull request Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.