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

3.0: Parametrization gets values from previous tests?! #1832

Closed
The-Compiler opened this Issue Aug 21, 2016 · 13 comments

Comments

Projects
None yet
6 participants
@The-Compiler
Member

The-Compiler commented Aug 21, 2016

So I just updated qutebrowser to pytest 3.0, and did run into a weird issue...

The regular build on AppVeyor works just fine.

For the frozen build tests seem to pick up parametrization values of previous tests?!

For example, near the bottom, this is failing:

tests\unit\config\test_style.py F
_____________________ TestColorDict.test_values[foo-one] ______________________
self = <test_style.TestColorDict object at 0x06FC1130>, key = 'foo'
expected = True
    @pytest.mark.parametrize('key, expected', [
        ('foo', 'one'),
        ('foo.fg', 'two'),
        ('foo.bg', 'three'),
    ])
    def test_values(self, key, expected):
        d = style.ColorDict()
        d['foo'] = 'one'
        d['foo.fg'] = 'two'
        d['foo.bg'] = 'three'
>       assert d[key] == expected
E       assert 'one' == True
tests\unit\config\test_style.py:102: AssertionError

Now where did that True for expected come from? It's certainly not in the parametrized list there...

Then for the next test, suddenly the 'foo' we parametrized for key shows up again:

_________________ TestKeyToString.test_normal[16777495-Blue] __________________
self = <test_utils.TestKeyToString object at 0x071731D0>, key = 'foo'
expected = True
    @pytest.mark.parametrize('key, expected', [
        (Qt.Key_Blue, 'Blue'),
        (Qt.Key_Backtab, 'Tab'),
        (Qt.Key_Escape, 'Escape'),
        (Qt.Key_A, 'A'),
        (Qt.Key_degree, '�'),
        (Qt.Key_Meta, 'Meta'),
    ])
    def test_normal(self, key, expected):
        """Test a special key where QKeyEvent::toString works incorrectly."""
>       assert utils.key_to_string(key) == expected
E       assert '' == True
E        +  where '' = <function key_to_string at 0x039CC810>('foo')
E        +    where <function key_to_string at 0x039CC810> = utils.key_to_string
tests\unit\utils\test_utils.py:451: AssertionError

here, key never should be 'foo' and expected never should be True...

@The-Compiler The-Compiler referenced this issue Aug 21, 2016

Closed

pytest 3.0 checklist #1877

11 of 11 tasks complete
@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 21, 2016

Trying to bisect this, but it'll probably take me a while...

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 21, 2016

That's weird... 😕

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 21, 2016

Bisected to d72afe7

cc @Stranger6667

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 21, 2016

So indeed reverting that commit on the newest feature branch fixes things.

Looking at that commit, it relies on the order of _arg2fixturedefs:

            if self._arg2fixturedefs:
                # Takes the most narrow scope from used fixtures
                fixtures_scopes = [fixturedef[0].scope for fixturedef in self._arg2fixturedefs.values()]
                for scope in reversed(scopes):
                    if scope in fixtures_scopes:
                        break
            else:
                scope = 'function'

However, self._args2fixturedefs is a dict (created in FixtureManager.getfixtureinfo). Dicts are unordered, so I'm not sure what the intention of this is?

@Stranger6667

This comment has been minimized.

Contributor

Stranger6667 commented Aug 21, 2016

Hi, the only thing here is to check if scope exists in defined fixtures. The order doesn't matter

@vmalloc

This comment has been minimized.

vmalloc commented Aug 21, 2016

In case it helps, here is an overly simplified case that reproduces this: https://github.com/vmalloc/pytest-bug-reproduction

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 21, 2016

@vmalloc Thanks, that definitely helps a lot! I can confirm that reproduces it.

@Stranger6667 Oh, I'm sorry - I misread reversed(scopes) as reversed(fixture_scopes). Do you have any idea what's causing this issue though? Because right now, I don't...

@vmalloc

This comment has been minimized.

vmalloc commented Aug 21, 2016

From what I've seen in my case, the bug appears only in the presence of the session-scoped fixture(s)

@Kingdread

This comment has been minimized.

Kingdread commented Aug 21, 2016

If no other fixture is used, the default scope is 'function', which is correct, but if at least one other fixture is used, the most narrow scope is used. If you have an autouse session-scoped fixture, and nothing else, session is actually the most narrow scope, which means even the single parameters are scoped as session, which may confuse pytest and cause them to appear in seemingly unrelated tests (I'm not that familar with the inner workings of this machinery...). So the key parameter of one function actually appears in another one.

This is "proven" correct by adding a single function-scoped fixture to the tests, without modifying (the bugged) pytest: suddenly, the tests are green again:

diff --git a/tests/test_front_matter_parser.py b/tests/test_front_matter_parser.py
index 96d5917..6a0214b 100644
--- a/tests/test_front_matter_parser.py
+++ b/tests/test_front_matter_parser.py
@@ -1,14 +1,18 @@
 import pytest

+@pytest.fixture(scope='function')
+def bugfix():
+    pass
+
 @pytest.mark.parametrize('animal', [
     "dog",
     "cat",
 ])
-def test_1(animal):
+def test_1(animal, bugfix):
     assert animal != 'fish'

 @pytest.mark.parametrize('animal', [
     'fish',
 ])
-def test_2(animal):
+def test_2(animal, bugfix):
     assert animal == 'fish'

-> 3 passed in 0.02 seconds

Now, as I said, I'm not familar with the inner workings of pytest, but I guess that the scope should stay "function" if at least one non-indirect parametrization is given (who wants a single parameter parametrized over a whole session?). A quick and dirty solution might look like

diff --git a/_pytest/python.py b/_pytest/python.py
index 7abd962..47da080 100644
--- a/_pytest/python.py
+++ b/_pytest/python.py
@@ -807,14 +807,26 @@ class Metafunc(fixtures.FuncargnamesCompatAttr):
             newmarks[newmark.markname] = newmark

         if scope is None:
-            if self._arg2fixturedefs:
-                # Takes the most narrow scope from used fixtures
-                fixtures_scopes = [fixturedef[0].scope for fixturedef in self._arg2fixturedefs.values()]
-                for scope in reversed(scopes):
-                    if scope in fixtures_scopes:
-                        break
-            else:
+            if indirect is False or isinstance(indirect, (tuple, list)) and len(indirect) < len(argnames):
+                # at least one non-indirect fixture, scope should be function
+                # then
                 scope = 'function'
+            else:
+                fixturedefs = self._arg2fixturedefs
+                if fixturedefs is None:
+                    fixturedefs = {}
+                used_scopes = [fixturedef[0].scope
+                               for name, fixturedef in fixturedefs.items()
+                               if indirect is True
+                               or isinstance(indirect, (tuple, list))
+                               and name in indirect]
+                if used_scopes:
+                    # Takes the most narrow scope from used fixtures
+                    for scope in reversed(scopes):
+                        if scope in used_scopes:
+                            break
+                else:
+                    scope = 'function'
         scopenum = scopes.index(scope)
         valtypes = {}
         for arg in argnames:

i.e. the scope stays "function" if at least one non-indirect parametrization is given, and only scopes actually parametrized are considered when searching the most narrow scope.

Now (after reverting the bugfix in the example), the tests for both the bug example and the example for issue 634 are green:

[daniel@persephone]  pytest-bug/pytest-bug-reproduction % git status       
On branch master
Your branch is up-to-date with 'origin/master'.
nothing to commit, working tree clean
[daniel@persephone]  pytest-bug/pytest-bug-reproduction % ../venv/bin/python -m pytest tests
====================================================================================== test session starts =======================================================================================
platform linux -- Python 3.5.2, pytest-3.0.1.dev, py-1.4.31, pluggy-0.3.1
rootdir: /home/daniel/Code/pytest-bug/pytest-bug-reproduction, inifile: 
plugins: hypothesis-3.4.2
collected 3 items 

tests/test_front_matter_parser.py ...

==================================================================================== 3 passed in 0.02 seconds ====================================================================================
[daniel@persephone]  pytest-bug/pytest-bug-reproduction % ../venv/bin/python -m pytest ../pytest/testing/python/metafunc.py 
====================================================================================== test session starts =======================================================================================
platform linux -- Python 3.5.2, pytest-3.0.1.dev, py-1.4.31, pluggy-0.3.1
rootdir: /home/daniel/Code/pytest-bug/pytest, inifile: tox.ini
plugins: hypothesis-3.4.2
collected 82 items 

../pytest/testing/python/metafunc.py .......................................................................x..........
==================================================================================== short test summary info =====================================================================================
XFAIL ../pytest/testing/python/metafunc.py::TestMarkersWithParametrization::()::test_nested_marks
  is this important to support??

============================================================================= 81 passed, 1 xfailed in 15.45 seconds =============================================================================

This might be a starting point for someone who knows more about pytest and how parametrize (should work|works).

@daroot

This comment has been minimized.

daroot commented Aug 21, 2016

I can confirm this behavior also exists with module scoped fixtures as well. As long as there are no explicit function scoped fixtures, parametrized fixtures get weird values from other tests.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Aug 22, 2016

I can confirm what @Kingdread says - I added a autouse function-scoped fixture to my conftest.py, and sure enough, my tests are passing again.

The-Compiler added a commit to qutebrowser/qutebrowser that referenced this issue Aug 22, 2016

wence- added a commit to firedrakeproject/firedrake that referenced this issue Aug 22, 2016

Pin pytest version at 2.9.x
Temporary workaround for pytest-dev/pytest#1832.
@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 22, 2016

A simple workaround is also to pass scope="function" to the parametrize call.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Aug 23, 2016

PR opened at #1847

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment