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

Ordering of session fixtures is wrong when combined with class or module fixtures under some conditions #3161

Open
ceridwen opened this Issue Jan 29, 2018 · 19 comments

Comments

Projects
None yet
4 participants
@ceridwen
Contributor

ceridwen commented Jan 29, 2018

This is a continuation of the discussion in #519 and #2959, and it may be related to one or both of those issues. I have a test suite where I have to run some very expensive configuration before running the entire test suite, and the set of possible configurations is the Cartesian product of modes that are fixed with flavors that I need to be able to pass in at run-time. Using the example @RonnyPfannschmidt provided in #519 with @popravich's edit and fixing indentation, I ended up with this code:

def pytest_configure(config):
    class DynamicFixturePlugin(object):
        @pytest.fixture(scope='session', params=config.getoption('--reprovision-flavors').split(','))
        def flavor(self, request):
            return request.param
    config.pluginmanager.register(DynamicFixturePlugin(), 'flavor-fixture')

@pytest.fixture(scope='session', params=['vxlan', pytest.mark.skipif("not pytest.config.getoption('--reprovision-fabric')")('vlan')])
def encap(request):
    return request.param

@pytest.fixture(scope='session', autouse='True')
def reprovision(request, flavor, encap):
    fabric_name = request.config.getoption('--reprovision-fabric')
    if fabric_name:
        kubernetes_.deploy(fabric_name, reinstall=request.config.getoption('--reprovision-reinstall'), encap=encap, **kubernetes_.FLAVORS[flavor])

This half-works. What happens is that for the first two flavor-encap combinations, foo and bar and vxlan, reprovision runs once for each full execution of the test suite. For the second two flavor-encap combinations, reprovision runs twice for every test, as if it was function-scoped rather than session-scoped. I tried this alternate form of the above code with the same result:

def pytest_configure(config):
    class DynamicFixturePlugin(object):
        @pytest.fixture(scope='session', params=config.getoption('--reprovision-flavors').split(','))
        def flavor(self, request):
            return request.param
    config.pluginmanager.register(DynamicFixturePlugin(), 'flavor-fixture')
    class DynamicFixturePlugin2(object):
        @pytest.fixture(scope='session', params=['vxlan', 'vlan'] if config.getoption('--reprovision-fabric') else [])
        def encap(self, request):
            return request.param
    config.pluginmanager.register(DynamicFixturePlugin2(), 'encap-fixture')

@pytest.fixture(scope='session', autouse='True')
def reprovision(request, flavor, encap):
    fabric_name = request.config.getoption('--reprovision-fabric')
    if fabric_name:
        kubernetes_.deploy(fabric_name, reinstall=request.config.getoption('--reprovision-reinstall'), encap=encap, **kubernetes_.FLAVORS[flavor])

This is similar to how pytest_generate_tests breaks session-scoped fixtures.

I'm assuming these are bugs. How hard do you think it will be to fix them? Can anyone point me to where I need to look to start, assuming it's possible?

This is with pytest 3.2.0 on Python3 and Mac OS X 10.12.6. I have pytest-notifier, pytest-html, and pytest-metadata installed.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Jan 31, 2018

Hey @ceridwen sorry for the delay.

I created a small example to reproduce your setup:

# conftest.py
import pytest

def pytest_configure(config):
    class DynamicFixturePlugin(object):
        @pytest.fixture(scope='session', params=['flavor1', 'flavor2'])
        def flavor(self, request):
            print('flavor created:', request.param)
            return request.param

    config.pluginmanager.register(DynamicFixturePlugin(), 'flavor-fixture')

@pytest.fixture(scope='session', params=['vxlan', 'vlan'])
def encap(request):
    print('encap created:', request.param)
    return request.param

@pytest.fixture(scope='session', autouse='True')
def reprovision(request, flavor, encap):
    print('reprovision created:', flavor, encap)

# test_foo.py
def test(reprovision):
    pass

def test2(reprovision):
    pass

Running this in the latest master I get (cleaning up the output a bit for better legibility):

.tmp\session-fix\test_foo.py::test[flavor1-vxlan] 
flavor created: flavor1
encap created: vxlan
reprovision created: flavor1 vxlan
PASSED

.tmp\session-fix\test_foo.py::test[flavor2-vxlan] 
flavor created: flavor2
reprovision created: flavor2 vxlan
PASSED

.tmp\session-fix\test_foo.py::test[flavor1-vlan] 
flavor created: flavor1
encap created: vlan
reprovision created: flavor1 vlan
PASSED

.tmp\session-fix\test_foo.py::test[flavor2-vlan]
flavor created: flavor2
reprovision created: flavor2 vlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor1-vlan]
flavor created: flavor1
reprovision created: flavor1 vlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor2-vlan]
flavor created: flavor2
reprovision created: flavor2 vlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor2-vxlan]
encap created: vxlan
reprovision created: flavor2 vxlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor1-vxlan]
flavor created: flavor1
reprovision created: flavor1 vxlan
PASSED

While in pytest 3.2.0 I get:

.tmp\session-fix\test_foo.py::test[flavor1-vxlan]
flavor created: flavor1
encap created: vxlan
reprovision created: flavor1 vxlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor1-vxlan] 
PASSED

.tmp\session-fix\test_foo.py::test[flavor2-vxlan] 
flavor created: flavor2
reprovision created: flavor2 vxlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor2-vxlan] 
PASSED

.tmp\session-fix\test_foo.py::test[flavor2-vlan]
encap created: vlan
reprovision created: flavor2 vlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor2-vlan] 
PASSED

.tmp\session-fix\test_foo.py::test[flavor1-vlan] 
flavor created: flavor1
reprovision created: flavor1 vlan
PASSED

.tmp\session-fix\test_foo.py::test2[flavor1-vlan] 
PASSED

One aspect of the fixture caching is that only a single instance of a fixture can exist at any given time; for example if test requires fixture foo with param=1, if another fixture requires foo with param=2 then it means that foo[1] will be destroyed and foo[2] will be created; this is by design to avoid keeping a large number of parametrized fixtures alive at the same time.

The output differs between pytest 3.2 and 3.4 because of #3108, which was an optimization for the collection phase but has changed the order of the tests in a way that (at least for this case) is resulting in much more fixtures being created/destroyed. I don't think that was intentional, and it seems no test caught that which is concerning.

cc @cheezman34

@cheezman34

This comment has been minimized.

cheezman34 commented Feb 1, 2018

Here's a prototype for a test to add to testing/python/fixture.py

I'm happy to take a crack at fixing the ordering when I have a second.

def test_dynamic_parametrized_ordering(self, testdir):
    testdir.makeini("""
        [pytest]
        console_output_style=classic
    """)
    testdir.makeconftest("""
        import pytest

        def pytest_configure(config):
            class DynamicFixturePlugin(object):
                @pytest.fixture(scope='session', params=['flavor1', 'flavor2'])
                def flavor(self, request):
                    print('flavor created:', request.param)
                    return request.param

            config.pluginmanager.register(DynamicFixturePlugin(), 'flavor-fixture')

        @pytest.fixture(scope='session', params=['vxlan', 'vlan'])
        def encap(request):
            print('encap created:', request.param)
            return request.param

        @pytest.fixture(scope='session', autouse='True')
        def reprovision(request, flavor, encap):
            print('reprovision created:', flavor, encap)
    """)
    testdir.makepyfile(test_mod1="""
        def test(reprovision):
            pass

        def test2(reprovision):
            pass
    """)
    result = testdir.runpytest("-v")
    result.stdout.fnmatch_lines("""
        test_mod1.py::test[flavor1-vxlan] PASSED
        test_mod1.py::test[flavor1-vxlan] PASSED
        test_mod1.py::test[flavor2-vxlan] PASSED
        test_mod1.py::test[flavor2-vxlan] PASSED
        test_mod1.py::test2[flavor2-vlan] PASSED
        test_mod1.py::test2[flavor2-vlan] PASSED
        test_mod1.py::test2[flavor1-vlan] PASSED
        test_mod1.py::test2[flavor1-vlan] PASSED
    """)
@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Feb 1, 2018

FWIW, here are the lightly edited logs from the real tests running on pytest 3.2. The dicts correspond to reprovision calls. @nicoddemus, it looks to me like my example is different from yours, because in yours all the combinations are only run once, for 4 total calls to reprovision? In my example, though, reprovision runs many more times than that. Let me try 3.4 and see what that does, though as you say the behavior is not desirable.

test_kubernetes.py
{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vxlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...
....
...
....
...
....
...
....
...
....
...
...
...
...
xxx
...
...
.
...ssss
.
.
.
.
.
.
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vxlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...
....
...
....
...
....
...
....
...
....
...
...
...
...
xxx
...
.......ssss
.
.
.
.
.
..{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
....
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
....
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
xxx{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
xxx{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
...{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
...{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
...ssssssss{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.{"sstage": 2, "flavor": "kube18", "tag": "kube18", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.8.7"}
.{"sstage": 2, "flavor": "kube17", "tag": "kube17", "mode": "vlan", "nobonds": false, "noha": true, "dry_run": true, "reinstall": false, "name": "name", "version": "1.7.12"}
.
@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Feb 1, 2018

As for @nicoddemus's example, in 3.4 the session-scoped fixtures run before every combination of parameters for every test, even for the parametrized tests that did not in the second run in my example (the vlan tests only run once for each test function, not each for parameter for each test function).

@cheezman34

This comment has been minimized.

cheezman34 commented Feb 1, 2018

FYI, I think I've figured out the ordering issue. Working on it now.

@cheezman34

This comment has been minimized.

cheezman34 commented Feb 1, 2018

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Feb 2, 2018

@cheezman34's pull request restores the pytest 3.2 behavior, which definitely helps, but there's still the question of how the dynamic fixture is interfering with the session-scoping of the fixtures such that in the second run it's switching back and forth between the flavors for every (parametrized) test, rather than running all of one flavor first and all of the other second, which would lead to the correct 4-times calls against the fixture. Is the ordering code the right place to look here, or should I be looking at something else?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Feb 2, 2018

Is the ordering code the right place to look here, or should I be looking at something else?

AFAIK the ordering code is the place to look.

Session-scoped fixtures are destroyed while the tests are running if another instance of it is required by a test because of parametrization, so the old instance is destroyed to give place for a new instance (with the new parameters). This is the reason why pytest tries to order the tests to keep session-scoped fixtures switching between each test to a minimal.

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Mar 12, 2018

I've upgraded to 3.4.2 for @cheezman34's fix to the regression. While trying to track down the cause of this bug, I stumbled across another bit of weirdness that may or may not be related: #3225 (comment)

ScopeMismatch: You tried to access the 'function' scoped fixture 'baz' with a 'session' scoped request object, involved factories
conftest.py:141:  def autouse_fixture_that_depends_on_dynamic_fixture(request, baz, ...)

The reason I was wondering if it was related is because the error message in that case suggests that pytest is incorrectly interpreting a session-scoped fixture as a function-scope one, which is what the behavior in this case suggests is going wrong. I could be wrong, though.

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Mar 13, 2018

On a further look, this is definitely an ordering problem, because running --collect-only,

<Module 'temp_pytest.py'>
  <Function 'test_read[namespace0-pod-kubernetes-1.7-vxlan]'>
  <Function 'test_read[namespace0-deployment-kubernetes-1.7-vxlan]'>
  <Function 'test_read[namespace0-service-kubernetes-1.7-vxlan]'>
  <Function 'test_read[namespace0-network_policy-kubernetes-1.7-vxlan]'>
  <Function 'test_read[namespace0-job-kubernetes-1.7-vxlan]'>
  <Function 'test_read[namespace0-secret-kubernetes-1.7-vxlan]'>
  <Function 'test_read[namespace0-pod-kubernetes-1.8-vxlan]'>
  <Function 'test_read[namespace0-deployment-kubernetes-1.8-vxlan]'>
  <Function 'test_read[namespace0-service-kubernetes-1.8-vxlan]'>
  <Function 'test_read[namespace0-network_policy-kubernetes-1.8-vxlan]'>
  <Function 'test_read[namespace0-job-kubernetes-1.8-vxlan]'>
  <Function 'test_read[namespace0-secret-kubernetes-1.8-vxlan]'>
  <Function 'test_read[namespace0-pod-kubernetes-1.8-vlan]'>
  <Function 'test_read[namespace0-pod-kubernetes-1.7-vlan]'>
  <Function 'test_read[namespace0-deployment-kubernetes-1.8-vlan]'>
  <Function 'test_read[namespace0-deployment-kubernetes-1.7-vlan]'>
  <Function 'test_read[namespace0-service-kubernetes-1.8-vlan]'>
  <Function 'test_read[namespace0-service-kubernetes-1.7-vlan]'>
  <Function 'test_read[namespace0-network_policy-kubernetes-1.8-vlan]'>
  <Function 'test_read[namespace0-network_policy-kubernetes-1.7-vlan]'>
  <Function 'test_read[namespace0-job-kubernetes-1.8-vlan]'>
  <Function 'test_read[namespace0-job-kubernetes-1.7-vlan]'>
  <Function 'test_read[namespace0-secret-kubernetes-1.8-vlan]'>
  <Function 'test_read[namespace0-secret-kubernetes-1.7-vlan]'>

Unfortunately, I haven't been able to find anything resembling a minimal example. Does anyone have suggestions as to what could be throwing off the ordering?

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Mar 13, 2018

This is probably not the most minimal possible example, but it's close:

# conftest.py
import pytest

@pytest.fixture(scope='session')
def instance(request):
    pass

def pytest_configure(config):
    class DynamicFixturePlugin(object):
        @pytest.fixture(scope='session', params=['1.7', '1.8'])
        def version(self, request):
            print('VERSION:', request.param)
            return request.param
    config.pluginmanager.register(DynamicFixturePlugin(), 'version-fixture')
    class DynamicFixturePlugin2(object):
        @pytest.fixture(scope='session', params=['vxlan', 'vlan'])
        def encap_mode(self, request):
            print('ENCAP:', request.param)
            return request.param
    config.pluginmanager.register(DynamicFixturePlugin2(), 'encap_mode-fixture')

@pytest.fixture(scope='session', autouse='True')
def reprovision(request, version, encap_mode):
    print('REPROVISION:', version, encap_mode)

# test_foo.py
import pytest

def closure(params):
    @pytest.fixture(scope='module', params=params)
    def _fixture(request):
        instance = request.getfixturevalue('instance')
        print('CREATED', request.param)
        yield request.param
        print('DELETED', request.param)
    return _fixture

namespace = closure(['baz'])
resource = closure(['foo', 'bar'])

def test(instance, namespace, resource):
    print(instance, namespace, resource)

The key, it turns out, is the scope of the fixtures in test_foo.py. If the scope is function, the ordering is correct. If it's class or module, it gives the wrong order that forces the session fixture to rerun after every test.

@ceridwen ceridwen changed the title from Scope of session fixtures broken when created dynamically to Ordering of session fixtures is wrong when combined with class or module fixtures under some conditions Mar 13, 2018

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Mar 13, 2018

After spending some time staring at the reorder_items_atscope function, I still don't understand what the order it's supposed to produce is. I would assume that the correct order is to group all the fixtures of a given scope together and otherwise preserve the order in which they're processed. Are there more constraints than that?

@cheezman34

This comment has been minimized.

cheezman34 commented Mar 14, 2018

Look at testing.python.fixture for test_module_parametrized_ordering to get a picture of expected output. I would try printing out "items" at the beginning of reorder_items_atscope and "items_done" at the end to figure out exactly what's happening. You might also look at what the code was before I made my changes (it might be easier to understand).

I honestly don't know whether it's an optimal algorithm or not for sorting (probably not), I just tried to replicate existing functionality when I made my changes.

@cheezman34

This comment has been minimized.

cheezman34 commented Mar 14, 2018

What would be your expected output from your previous example versus what it actually gives you?

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Mar 14, 2018

@cheezman34, you successfully replicated the 3.2 ordering :). I'm saying that the 3.2 order is wrong. This is the order with class- or module-scoped fixtures in my toy example:

  <Function 'test[baz-foo-1.7-vxlan]'>
  <Function 'test[baz-bar-1.7-vxlan]'>
  <Function 'test[baz-foo-1.8-vxlan]'>
  <Function 'test[baz-bar-1.8-vxlan]'>
  <Function 'test[baz-foo-1.8-vlan]'>
  <Function 'test[baz-foo-1.7-vlan]'>
  <Function 'test[baz-bar-1.8-vlan]'>
  <Function 'test[baz-bar-1.7-vlan]'>

This is the order with function-scoped fixtures in my toy example:

  <Function 'test[baz-foo-1.7-vxlan]'>
  <Function 'test[baz-bar-1.7-vxlan]'>
  <Function 'test[baz-foo-1.8-vxlan]'>
  <Function 'test[baz-bar-1.8-vxlan]'>
  <Function 'test[baz-foo-1.8-vlan]'>
  <Function 'test[baz-bar-1.8-vlan]'>
  <Function 'test[baz-foo-1.7-vlan]'>
  <Function 'test[baz-bar-1.7-vlan]'>

The function-scoped fixtures have the right order, the class- and module-scoped fixtures have the wrong order. It's wrong because it forces the expensive session fixture to run two additional times. (In the real test suite, it runs after every test, which makes a test suite that should take an hour at most take more than a day. I've never actually run it to completion, it takes too long.). The initial order for the fixtures after collection but before reorder_items_atscope() runs is:

OrderedDict([(<Function 'test[baz-foo-1.7-vxlan]'>, None),
             (<Function 'test[baz-foo-1.7-vlan]'>, None),
             (<Function 'test[baz-foo-1.8-vxlan]'>, None),
             (<Function 'test[baz-foo-1.8-vlan]'>, None),
             (<Function 'test[baz-bar-1.7-vxlan]'>, None),
             (<Function 'test[baz-bar-1.7-vlan]'>, None),
             (<Function 'test[baz-bar-1.8-vxlan]'>, None),
             (<Function 'test[baz-bar-1.8-vlan]'>, None)])

The after order is the same as given above.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 14, 2018

@ceridwen I have to take a closer look, but running your toy example with branch from #3306 I get:

.tmp\param_order\test_foo.py::test[1.7-vxlan-baz-foo] PASSED                                                     [ 12%]
.tmp\param_order\test_foo.py::test[1.7-vxlan-baz-bar] PASSED                                                     [ 25%]
.tmp\param_order\test_foo.py::test[1.8-vxlan-baz-foo] PASSED                                                     [ 37%]
.tmp\param_order\test_foo.py::test[1.8-vxlan-baz-bar] PASSED                                                     [ 50%]
.tmp\param_order\test_foo.py::test[1.8-vlan-baz-foo] PASSED                                                      [ 62%]
.tmp\param_order\test_foo.py::test[1.7-vlan-baz-foo] PASSED                                                      [ 75%]
.tmp\param_order\test_foo.py::test[1.8-vlan-baz-bar] PASSED                                                      [ 87%]
.tmp\param_order\test_foo.py::test[1.7-vlan-baz-bar] PASSED                                                      [100%]

The last 4 runs (with vlan) don't look right to me, they should have swapped the middle items like this:

.tmp\param_order\test_foo.py::test[1.8-vlan-baz-foo] PASSED
.tmp\param_order\test_foo.py::test[1.8-vlan-baz-bar] PASSED
.tmp\param_order\test_foo.py::test[1.7-vlan-baz-foo] PASSED
.tmp\param_order\test_foo.py::test[1.7-vlan-baz-bar] PASSED
@cheezman34

This comment has been minimized.

cheezman34 commented Mar 14, 2018

I think this might be your problem:

The sorting algorithm treats all fixtures of a given scope as having the same priority, which means some test configuration will have multiple valid orders. For example, given session-scoped fixtures A1, A2, B1, and B2, and tests that run on a cartesian product of these:

(A1, B1)
(A1, B2)
(A2, B2)
(A2, B1)

...is a valid order.

(A1, B1)
(A2, B1)
(A2, B2)
(A1, B2)

...is also a valid order. The algorithm doesn't really care which one it produces.

If, however, A is session-scoped, and B is module-scoped, then only the first order is valid, because session-scoped fixtures have a higher sort priority than module-scoped. In your toy example, both your expected output and the received output are valid orderings, because the total number of fixture creations is the same (the first has one less foo/bar transition and one more 1.7/1.8 transition when compared to the second).

Basically, pytest has no idea that it should prioritize reprovision before version & encap-mode when sorting, because it assumes they all have equal sort priority.

@ceridwen

This comment has been minimized.

Contributor

ceridwen commented Mar 14, 2018

@nicoddemus, The last 4 runs are the ones that are wrong, yes.

@cheezman34, The algorithm doesn't care about the ordering of session fixtures, and neither do I. Either of the two orders you give are valid, if only session fixtures are involved. The problem is when you add a class or module fixture, it effectively receives priority over one of the session fixtures in ordering. If I pick the first of your valid orders and add a fixture, this is a valid order:

(A1, B1, C1)
(A1, B1, C2)
(A1, B2, C1)
(A1, B2, C2)
(A2, B2, C1)
(A2, B2, C2)
(A2, B1, C1)
(A2, B1, C2)

As you pointed out, there's another valid order too. Instead, I'm getting the invalid order:

(A1, B1, C1)
(A1, B1, C2)
(A1, B2, C1)
(A1, B2, C2)
(A2, B2, C1)
(A2, B1, C1)
(A2, B2, C2)
(A2, B1, C2)

Here, if C is module- or class-scoped, the order is wrong for last four runs (the first four runs have the right order). If C is function-scoped, the order is right for all the runs.

@Zac-HD

This comment has been minimized.

Member

Zac-HD commented Oct 19, 2018

#3306 fixed most of this issue, but the ordering can still be broken later when autouse fixtures are applied.

@Zac-HD Zac-HD removed the type: question label Oct 19, 2018

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