Fix #3854 #3861

jonozzz · 2018-08-23T04:49:28Z

If a test was found in a file that started with a capital letter within a subpackage of a package pytest would collect it twice.

Fix #3796, fix #3854

jonozzz · 2018-08-23T18:08:22Z

I'm not sure why it doesn't load the config from the generated pytest.ini and instead it picks tox.ini. Should I pass it explicitly as an argument -c pytest.ini ?

nicoddemus · 2018-08-24T01:03:23Z

I'm not sure why it doesn't load the config from the generated pytest.ini and instead it picks tox.ini. Should I pass it explicitly as an argument -c pytest.ini ?

Sorry what do you mean? You are not talking about the failing CI right, because it failed during linting.

jonozzz · 2018-08-24T01:31:33Z

I'm talking about the AppVeyor CI which failed test_package_ordering. It's as if the ini file that the tests generates was ignored.

nicoddemus · 2018-08-24T01:51:57Z

Oh sorry did not realize that, I only looked at Travis.

It is confusing (and I just noticed this now), but makeini creates a tox.ini file:

pytest/src/_pytest/pytester.py

Lines 578 to 580 in f2e35c8

    
           def makeini(self, source): 
        
               """Write a tox.ini file with 'source' as contents.""" 
        
               return self.makefile(".ini", tox=source)

So it seems it is picking up the correct file:

---------------------------- Captured stdout call -----------------------------
============================= test session starts =============================
platform win32 -- Python 3.7.0, pytest-3.7.3.dev50+ga3060e45, py-1.5.4, pluggy-0.7.1 -- c:\projects\pytest\.tox\py37\scripts\python.exe
cachedir: .pytest_cache
rootdir: C:\Users\appveyor\AppData\Local\Temp\1\pytest-of-appveyor\pytest-0\test_package_ordering0, inifile: tox.ini
plugins: hypothesis-3.69.1
collecting ... collected 4 items

jonozzz · 2018-08-24T02:03:07Z

Ok, then why is it disregarding the python_files option from the ini file. I'm still confused... It's working fine when I run it locally.

nicoddemus · 2018-08-24T02:08:44Z

Please try to fix the linting issue so Travis can execute the rest of the jobs. This way we will know if it is a windows only issue or a difference between CI and your setup.

I'll be glad to help out with Windows if I can. 👍

jonozzz · 2018-08-24T05:08:29Z

$ pre-commit run --all-files
black....................................................................Failed
hookid: black

Uhm, I'm not sure what's black and why it failed 😄

jonozzz · 2018-08-24T06:12:28Z

testing/python/fixture.py:4001:13: F821 undefined name 'dedent'
How can it not be defined when it's clearly imported and even used by other tests. I'm confused...

nicoddemus · 2018-08-24T17:27:13Z

I'm not sure what's black and why it failed

black is our code formatter. 😉

Take a look at the contribution guide on how to install pre-commit so all of our checks happen on each commit. 👍

jonozzz · 2018-08-24T17:32:50Z

Ok, done that, I did a quick tox -e linting, and still nothing.

  linting: commands succeeded
  congratulations :)

nicoddemus · 2018-08-24T18:07:10Z

Ok, done that, I did a quick tox -e linting, and still nothing.

Strange, that usually does the trick for me... I will check your branch later when I get home. 👍

Meanwhile if @asottile has some time to help it would be great as well.

asottile · 2018-08-24T18:33:35Z

Implicit conflict with master, I changed the import in #3864

merge / rebase and you should be good!

coveralls · 2018-08-24T19:22:13Z

Coverage increased (+0.05%) to 92.639% when pulling c336449 on jonozzz:fix-3854 into c829061 on pytest-dev:master.

nicoddemus

Great work @jonozzz! Please take a look at my comments and see what you think.

Also please add a CHANGELOG entry for the fix. 👍

nicoddemus · 2018-08-24T20:59:25Z

testing/python/fixture.py

+            def fix():
+                values.append("pre-sub1")
+                yield values
+                values.pop()


Perhaps this should be

assert values.pop() == "pre-sub1"

for completeness?

nicoddemus · 2018-08-24T21:00:03Z

testing/python/fixture.py

+            def fix():
+                values.append("pre-sub2")
+                yield values
+                values.pop()


Perhaps this should be

assert values.pop() == "pre-sub2"

for completeness?

nicoddemus · 2018-08-24T21:00:47Z

testing/python/fixture.py

+        sub2.ensure("__init__.py")
+        sub2.join("conftest.py").write(
+            textwrap.dedent(
+                """\


To avoid this weird line break by black I usually put the triple strings into a variable:

contents = dedent("""\ ... """) sub2.join("conftest.py").write(contents)

But really up to you, just in case this bothers you. 😉

black is going to reformat that to:

contents = dedent( """\ ... """ )

:rip:

you can do this:

x = """\ ... """ print(dedent(x))

but then. meh.

jonozzz · 2018-08-24T22:53:52Z

@nicoddemus any idea what's up with the failures in the windows CI?

nicoddemus · 2018-08-24T23:02:36Z

test_package_ordering is failing because windows is case-insensitive, so Test* is matching test_in_sub2.py. I suggest to use a different prefix, check_* for example.

Don't know why TestScopeOrdering.test_multiple_packages is failing though...

nicoddemus · 2018-08-24T23:08:51Z

I printed the yielded objects in Package.collect:

<Package 'c:\\pytest\\.tmp\\multiple-packages\\root\\sub1'>
<Package 'c:\\pytest\\.tmp\\multiple-packages\\root\\sub2'>
<Module 'test_1.py'>
<Module 'test_2.py'>

This seems correct to me...

nicoddemus · 2018-08-24T23:16:34Z

Pushed a fix for test_package_ordering, will take a look at TestScopeOrdering.test_multiple_packages when I have sometime tomorrow. 👍

…ames. The test doesn't make sense for Windows, because of its case-insensitivity.

nicoddemus · 2018-08-24T23:27:36Z

@jonozzz I noticed you pushed another commit to fix test_package_ordering... did you see my previous commit?

jonozzz · 2018-08-24T23:28:34Z

Well, the whole point of the test was to have a test filename with capital letters. I'd keep all files that start with 'Test'.

nicoddemus · 2018-08-24T23:43:16Z

Well, the whole point of the test was to have a test filename with capital letters. I'd keep all files that start with 'Test'.

Oh I did not realize that, thanks!

jonozzz · 2018-08-24T23:44:58Z

I was able to repro the other issue on a local windows machine. I'm looking into it, but I'm still confused.

jonozzz · 2018-08-25T01:03:44Z

Finally! I hope I squashed that bug. Turns out this new test uncovered an older issue. Phew!

nicoddemus · 2018-08-25T01:06:04Z

I tested locally and indeed it works, great job!

Just need to fix linting now hehehe.

asottile · 2018-08-25T01:06:24Z

@jonozzz btw if you run pre-commit install locally it'll let you know as you commit :D

jonozzz · 2018-08-25T01:08:21Z

Got too excited and pushed it without checking.
Thanks @asottile I need to play around with these hooks.

nicoddemus · 2018-08-25T01:08:53Z

@jonozzz btw if you add "Fix #NUMBER" to the PR or to one of the commits, the issue will be closed automatically when merged (I just updated the PR description adding that). You can see more here: https://help.github.com/articles/closing-issues-using-keywords/

jonozzz · 2018-08-25T01:22:34Z

Thanks. Everyday I learn new things 😁 I hope next PRs won't be as messy as this one.

nicoddemus · 2018-08-25T01:29:05Z

I hope next PRs won't be as messy as this one.

Not at all, anyway it is all part of the process, and we really appreciate all your efforts on this front! 👍

turturica added 3 commits August 9, 2018 18:06

Collect any tests from a package's __init__.py

d0bd01b

Merge branch 'master' of github.com:pytest-dev/pytest into fix-3854

e4f76f6

Fixes pytest-dev#3854

4d3c1ab

jonozzz requested a review from nicoddemus August 23, 2018 04:49

turturica added 2 commits August 23, 2018 22:48

Address pytest-dev#3796 and add a test for it.

5f8b50c

Make linting happy.

72e6482

turturica added 2 commits August 24, 2018 11:47

Merge branch 'master' of github.com:pytest-dev/pytest into fix-3854

3396225

Fix dedent after merge.

459b040

Add encoding: utf8 for python 2.7

e3df103

nicoddemus requested changes Aug 24, 2018

View reviewed changes

Added changelog items.

dce8df4

Fix test_package_ordering on Windows

f0226e9

test_package_ordering: Collect *.py, but keep a mix of case for filen…

8cf0e46

…ames. The test doesn't make sense for Windows, because of its case-insensitivity.

Fix the package fixture ordering in Windows.

1e4ecda

Make linting happy. Argh.

c336449

nicoddemus approved these changes Aug 25, 2018

View reviewed changes

nicoddemus merged commit be4b359 into pytest-dev:master Aug 25, 2018

nicoddemus mentioned this pull request Aug 25, 2018

Collect any tests from a package's __init__.py #3797

Merged

Fix #3854 #3861

Fix #3854 #3861

Conversation

jonozzz commented Aug 23, 2018 • edited by nicoddemus Loading

jonozzz commented Aug 23, 2018

nicoddemus commented Aug 24, 2018

jonozzz commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

jonozzz commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

jonozzz commented Aug 24, 2018

jonozzz commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

jonozzz commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

asottile commented Aug 24, 2018

coveralls commented Aug 24, 2018 • edited Loading

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus Aug 24, 2018

Choose a reason for hiding this comment

nicoddemus Aug 24, 2018

Choose a reason for hiding this comment

nicoddemus Aug 24, 2018

Choose a reason for hiding this comment

asottile Aug 24, 2018

Choose a reason for hiding this comment

nicoddemus Aug 24, 2018

Choose a reason for hiding this comment

asottile Aug 24, 2018

Choose a reason for hiding this comment

jonozzz commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

jonozzz commented Aug 24, 2018

nicoddemus commented Aug 24, 2018

jonozzz commented Aug 24, 2018

jonozzz commented Aug 25, 2018

nicoddemus commented Aug 25, 2018

asottile commented Aug 25, 2018

jonozzz commented Aug 25, 2018

nicoddemus commented Aug 25, 2018

jonozzz commented Aug 25, 2018

nicoddemus commented Aug 25, 2018

jonozzz commented Aug 23, 2018 •

edited by nicoddemus

Loading

coveralls commented Aug 24, 2018 •

edited

Loading