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

Fix incorrect discovery of non-test __init__.py files. #6197

Merged
merged 2 commits into from Nov 15, 2019

Conversation

asottile
Copy link
Member

@asottile asottile commented Nov 15, 2019

Resolves #6194

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

Simpler patch:

This fixes the regression:
```diff
@ src/_pytest/python.py:643 @ def isinitpath(self, path):
         return path in self.session._initialpaths

     def collect(self):
-        self._mount_obj_if_needed()
         this_path = self.fspath.dirpath()
         init_module = this_path.join("__init__.py")
         if init_module.check(file=1) and path_matches_patterns(
             init_module, self.config.getini("python_files")
         ):
+            self._mount_obj_if_needed()
             yield Module(init_module, self)
         pkg_prefixes = set()
         for path in this_path.visit(rec=self._recurse, bf=True, sort=True):

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

When reverting (which I am not opposed to, I think it's rather a feature not a bugfix in the first place), it should have a changelog for that.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Nov 15, 2019

If @blueyed's patch fixes the regression, we probably should go with that instead (perhaps makes sense to open a separate PR?).

Can somebody prepare a release then? I won't be on my computer later.

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

I can do it, I'll rebase that into this

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

When reverting (which I am not opposed to, I think it's rather a feature not a bugfix in the first place), it should have a changelog for that.

A revert of a bugfix which causes a regression is not a feature -- there is a changelog for the bugfix though

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

A revert of a bugfix which causes a regression is not a feature

Never said that - I've meant the original bugfix was more like a feature.

there is a changelog for the bugfix though

Cool, but in case you had kept the revert this would have needed a changelog entry from what I can tell. That's what I've said.

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

ok, that patch "fixes" the tests I added but breaks the feature in the original patch -- I'm going to go back to fully reverting in spirit of getting this fix out

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

ok, we're back to revert + new tests now 🎉

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

This fixes the test:

diff --git c/testing/test_skipping.py i/testing/test_skipping.py
index 371c3a4db..37b38b59c 100644
--- c/testing/test_skipping.py
+++ i/testing/test_skipping.py
@@ -1174,7 +1174,6 @@ def test_skip_package(testdir):

     testdir.makepyfile(
         """
-        import pytest
         def test_skip1():
             assert 0
         def test_skip2():
@@ -1182,6 +1181,6 @@ def test_skip2():
     """
     )

-    result = testdir.inline_run()
+    result = testdir.inline_run("-o", "python_files=*.py")
     _, skipped, _ = result.listoutcomes()
     assert len(skipped) == 2

I'm not sure, but in general python_files needs to include __init__.py for them / packages to be cosidered.
/cc @zefirior

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

I'd rather revert at this point

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

I'm not opposed to it still. Please merge master into features then though afterwards.

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

of course, that's part of releasing after all :)

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

could someone review this patch or should I just take it out?

@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

Let's wait for @nicoddemus or @RonnyPfannschmidt - since I think the initial test might be wrong I am not approving it myself (but also not blocking it of course).

changelog/6197.bugfix.rst Outdated Show resolved Hide resolved
@asottile asottile merged commit 19a15a9 into pytest-dev:master Nov 15, 2019
@asottile asottile deleted the fix_init_py_discovery branch Nov 15, 2019
@blueyed
Copy link
Contributor

@blueyed blueyed commented Nov 15, 2019

Can you create the PR for merging master into features then, please?

@asottile
Copy link
Member Author

@asottile asottile commented Nov 15, 2019

yep, my computer was rebooting

bors bot added a commit to duckinator/bork that referenced this issue Nov 17, 2019
76: Update pytest to 5.2.4 r=duckinator a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **5.2.2** to **5.2.4**.



<details>
  <summary>Changelog</summary>
  
  
   ### 5.2.4
   ```
   =========================

Bug Fixes
---------

- `6194 &lt;https://github.com/pytest-dev/pytest/issues/6194&gt;`_: Fix incorrect discovery of non-test ``__init__.py`` files.


- `6197 &lt;https://github.com/pytest-dev/pytest/issues/6197&gt;`_: Revert &quot;The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.&quot;.
   ```
   
  
  
   ### 5.2.3
   ```
   =========================

Bug Fixes
---------

- `5830 &lt;https://github.com/pytest-dev/pytest/issues/5830&gt;`_: The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.


- `6099 &lt;https://github.com/pytest-dev/pytest/issues/6099&gt;`_: Fix ``--trace`` when used with parametrized functions.


- `6183 &lt;https://github.com/pytest-dev/pytest/issues/6183&gt;`_: Using ``request`` as a parameter name in ``pytest.mark.parametrize`` now produces a more
  user-friendly error.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
bors bot added a commit to rehandalal/therapist that referenced this issue Nov 19, 2019
103: Update pytest to 5.2.4 r=rehandalal a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **5.2.2** to **5.2.4**.



<details>
  <summary>Changelog</summary>
  
  
   ### 5.2.4
   ```
   =========================

Bug Fixes
---------

- `6194 &lt;https://github.com/pytest-dev/pytest/issues/6194&gt;`_: Fix incorrect discovery of non-test ``__init__.py`` files.


- `6197 &lt;https://github.com/pytest-dev/pytest/issues/6197&gt;`_: Revert &quot;The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.&quot;.
   ```
   
  
  
   ### 5.2.3
   ```
   =========================

Bug Fixes
---------

- `5830 &lt;https://github.com/pytest-dev/pytest/issues/5830&gt;`_: The first test in a package (``__init__.py``) marked with ``pytest.mark.skip`` is now correctly skipped.


- `6099 &lt;https://github.com/pytest-dev/pytest/issues/6099&gt;`_: Fix ``--trace`` when used with parametrized functions.


- `6183 &lt;https://github.com/pytest-dev/pytest/issues/6183&gt;`_: Using ``request`` as a parameter name in ``pytest.mark.parametrize`` now produces a more
  user-friendly error.
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Changelog: https://pyup.io/changelogs/pytest/
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
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.

3 participants