Fix nondeterminism in fixture collection order #2617

wence- · 2017-07-25T19:12:19Z

fixtures.reorder_items is non-deterministic because it reorders based
on iteration over an (unordered) set. Change the code to use an
OrderedDict instead, so that we get deterministic behaviour.

The xdist issue in #920 is a red-herring. Fixture collection is non-deterministic across pytest invocations full stop if hash randomisation is on.

For example, consider the following test file:

import pytest

@pytest.fixture(scope="module",
                params=["A",
                        "B",
                        "C"])
def A(request):
    return request.param


@pytest.fixture(scope="module",
                params=["DDDDDDDDD", "EEEEEEEEEEEE", "FFFFFFFFFFF", "GGGGGGGGGGGG"])
def B(request, A):
    return request.param


def test_foo(B):
    assert True

$pytest test_foo.py --collect-only
=============================================================================================== test session starts ================================================================================================
platform linux -- Python 3.5.2, pytest-3.1.3, py-1.4.34, pluggy-0.4.0
rootdir: /data/lmitche1/src/doodles, inifile:
plugins: xdist-1.18.1
collected 12 items 
<Module 'test_foo.py'>
  <Function 'test_foo[DDDDDDDDD-A]'>
  <Function 'test_foo[DDDDDDDDD-B]'>
  <Function 'test_foo[EEEEEEEEEEEE-B]'>
  <Function 'test_foo[EEEEEEEEEEEE-A]'>
  <Function 'test_foo[EEEEEEEEEEEE-C]'>
  <Function 'test_foo[DDDDDDDDD-C]'>
  <Function 'test_foo[FFFFFFFFFFF-C]'>
  <Function 'test_foo[FFFFFFFFFFF-B]'>
  <Function 'test_foo[FFFFFFFFFFF-A]'>
  <Function 'test_foo[GGGGGGGGGGGG-C]'>
  <Function 'test_foo[GGGGGGGGGGGG-B]'>
  <Function 'test_foo[GGGGGGGGGGGG-A]'>

=========================================================================================== no tests ran in 0.01 seconds ===========================================================================================
$ pytest test_foo.py --collect-only
=============================================================================================== test session starts ================================================================================================
platform linux -- Python 3.5.2, pytest-3.1.3, py-1.4.34, pluggy-0.4.0
rootdir: /data/lmitche1/src/doodles, inifile:
plugins: xdist-1.18.1
collected 12 items 
<Module 'test_foo.py'>
  <Function 'test_foo[DDDDDDDDD-A]'>
  <Function 'test_foo[EEEEEEEEEEEE-A]'>
  <Function 'test_foo[EEEEEEEEEEEE-B]'>
  <Function 'test_foo[DDDDDDDDD-B]'>
  <Function 'test_foo[FFFFFFFFFFF-B]'>
  <Function 'test_foo[FFFFFFFFFFF-A]'>
  <Function 'test_foo[FFFFFFFFFFF-C]'>
  <Function 'test_foo[EEEEEEEEEEEE-C]'>
  <Function 'test_foo[DDDDDDDDD-C]'>
  <Function 'test_foo[GGGGGGGGGGGG-C]'>
  <Function 'test_foo[GGGGGGGGGGGG-B]'>
  <Function 'test_foo[GGGGGGGGGGGG-A]'>

=========================================================================================== no tests ran in 0.01 seconds ===========================================================================================

Note how successive incantations of pytest arrive at different orders of the collected tests.

The problem lies in fixtures.reorder_items which reorders (in part) based on iteration over an unordered set. Fix this by switching to using an OrderedDict.

It's not obvious to me how to write a good test for this.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

Add a new news fragment into the changelog folder
- name it $issue_id.$type for example (588.bug)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of removal, feature, bugfix, vendor, doc or trivial
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

Add yourself to AUTHORS;

The-Compiler · 2017-07-25T19:22:18Z

This will probably break on Python 2.6, as collections.OrderedDict was added in 2.7.

coveralls · 2017-07-25T19:40:45Z

Coverage increased (+0.0009%) to 92.057% when pulling 8ff10ee on wence-:fix/nondeterministic-fixtures into 79097e8 on pytest-dev:master.

nicoddemus · 2017-07-25T23:26:17Z

Thanks for the PR!

I'm not sure how to easily fix the OrderedDict issue, suggestions are welcome.

About the test, I suggest to use the testdir fixture and inline_genitems() to get the list of collected items and ensure this is called with hash randomization on.

RonnyPfannschmidt · 2017-07-26T08:55:20Z

@nicoddemus this test will always have to use subprocesses as hash randomizarion is on a per process basis

wence- · 2017-07-26T09:14:33Z

I am about to push updates with test.

wence- · 2017-07-26T10:00:43Z

Added a test, and required ordereddict from pypi for py2.6.

Although the test I posted in the original comment reliably fails with different values of PYTHONHASHSEED, it does not reliably fail when run as a subprocess from the full pytest test suite (when providing the same PYTHONHASHSEED variation to the subprocesses). I cannot understand how this might happen.

The-Compiler · 2017-07-26T10:04:02Z

I'm okay with adding the dependency, but that should definitely be noted in the changelog, and the PR should go to the features branch.

coveralls · 2017-07-26T10:26:18Z

Coverage decreased (-0.02%) to 92.038% when pulling 2d2e97b on wence-:fix/nondeterministic-fixtures into 79097e8 on pytest-dev:master.

coveralls · 2017-07-26T10:49:30Z

Coverage decreased (-0.02%) to 92.038% when pulling 2d2e97b on wence-:fix/nondeterministic-fixtures into 79097e8 on pytest-dev:master.

wence- · 2017-07-26T13:00:34Z

Added new dependency for py26 in both changelog and docs/getting-started.rst. Retargeted to features rather than master.

Is there a policy on python2.6 support? I note that the last ever bug-fix release was October 2013.

nicoddemus · 2017-07-26T13:15:13Z

I'm fine with depending on ordereddict for py26, we already do the same with argparse.

Is there a policy on python2.6 support? I note that the last ever bug-fix release was October 2013.

We plan to drop it as soon as pip does in pip 10.0, see #1273.

nicoddemus

Thanks a lot!

I commented on some small nitpicks below.

nicoddemus · 2017-07-26T13:17:36Z

_pytest/fixtures.py

@@ -1,6 +1,12 @@
 from __future__ import absolute_import, division, print_function
 import sys

+try:
+    from collections import OrderedDict


Please check the version explicitly instead of relying on ImportError:

if sys.version_info[:2] == (2, 6): from ordereddict import OrderedDict else: from collections import OrderedDict

Using ImportError for that may lead to nasty surprises (see pytest-dev/pytest-mock#68 for a detailed example), plus this will make it easier for us later to find it and remove it when we drop 2.6 support.

Done. I guess it could go into _pytest/compat.py instead of here?

Done, I guess this import could go into _pytest/compat.py rather than here?

Good idea, this could be:

from _pytest.compat import OrderedDict

👍

nicoddemus · 2017-07-26T13:19:31Z

testing/python/fixture.py

@@ -2547,6 +2548,44 @@ def test_foo(fix):
            '*test_foo*alpha*',
            '*test_foo*beta*'])

+    @pytest.mark.issue920
+    def test_deterministic_fixture_collection(self, testdir):


Please use the monkeypatch fixture instead of importing and creating your own MonkeyPatch instance.

nicoddemus · 2017-07-26T13:22:58Z

testing/python/fixture.py

+        monkeypatch = MonkeyPatch()
+        monkeypatch.setenv("PYTHONHASHSEED", "1")
+        out1 = testdir.runpytest_subprocess("-v")
+        monkeypatch.undo()


No need to undo here

nicoddemus · 2017-07-26T13:23:14Z

testing/python/fixture.py

+        monkeypatch.undo()
+        monkeypatch.setenv("PYTHONHASHSEED", "2")
+        out2 = testdir.runpytest_subprocess("-v")
+        monkeypatch.undo()


Nor here, the monkeypatch fixture ensures to undo itself when the test ends.

Thanks, done.

nicoddemus · 2017-07-26T13:23:30Z

testing/python/fixture.py

@@ -2547,6 +2548,44 @@ def test_foo(fix):
            '*test_foo*alpha*',
            '*test_foo*beta*'])

+    @pytest.mark.issue920
+    def test_deterministic_fixture_collection(self, testdir):
+        a = testdir.mkdir("a")


Not sure you need the sub directory, plus you can ask testdir to create the file for you:

testdir.makepyfile('''' import pytest @pytest.fixture(scope="module", ...

This will create a test file named test_deterministic_fixture_collection.py (same as the test name) in the test directory.

Ah, I see, I was cargo-culting from the wrong place.

coveralls · 2017-07-26T13:39:51Z

Coverage decreased (-0.02%) to 91.899% when pulling b6ead46 on wence-:fix/nondeterministic-fixtures into 309152d on pytest-dev:features.

fixtures.reorder_items is non-deterministic because it reorders based on iteration over an (unordered) set. Change the code to use an OrderedDict instead so that we get deterministic behaviour, fixes #920.

nicoddemus

Awesome thanks @wence- !

That last bit about _pytest.compat is up to you and minor, it can be merged as soon as CI passes IMO.

coveralls · 2017-07-26T14:36:39Z

Coverage decreased (-0.008%) to 91.909% when pulling f047e07 on wence-:fix/nondeterministic-fixtures into 309152d on pytest-dev:features.

wence- · 2017-07-26T15:19:55Z

That last bit about _pytest.compat is up to you and minor,

I'm happy to leave as is.

wence- mentioned this pull request Jul 25, 2017

Python 3 migration firedrakeproject/firedrake#1108

Closed

13 tasks

wence- changed the base branch from master to features July 26, 2017 12:56

nicoddemus reviewed Jul 26, 2017

View reviewed changes

Add test of issue #920

b39f957

wence- added 3 commits July 26, 2017 14:41

Fix nondeterminism in fixture collection order

a546a61

fixtures.reorder_items is non-deterministic because it reorders based on iteration over an (unordered) set. Change the code to use an OrderedDict instead so that we get deterministic behaviour, fixes #920.

Add ordereddict to install_requires for py26

f8bd693

Mention new (py26) ordereddict dependency in changelog and docs

f047e07

nicoddemus approved these changes Jul 26, 2017

View reviewed changes

nicoddemus mentioned this pull request Jul 26, 2017

New scheduler for distribution of groups of related tests pytest-dev/pytest-xdist#191

Merged

nicoddemus merged commit 4cd8727 into pytest-dev:features Jul 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix nondeterminism in fixture collection order #2617

Fix nondeterminism in fixture collection order #2617

wence- commented Jul 25, 2017 •

edited

Loading

The-Compiler commented Jul 25, 2017

coveralls commented Jul 25, 2017

nicoddemus commented Jul 25, 2017

RonnyPfannschmidt commented Jul 26, 2017

wence- commented Jul 26, 2017

wence- commented Jul 26, 2017

The-Compiler commented Jul 26, 2017

coveralls commented Jul 26, 2017

coveralls commented Jul 26, 2017

wence- commented Jul 26, 2017

nicoddemus commented Jul 26, 2017

nicoddemus left a comment

nicoddemus Jul 26, 2017

wence- Jul 26, 2017

wence- Jul 26, 2017

nicoddemus Jul 26, 2017

nicoddemus Jul 26, 2017

wence- Jul 26, 2017

nicoddemus Jul 26, 2017

nicoddemus Jul 26, 2017

wence- Jul 26, 2017

nicoddemus Jul 26, 2017

wence- Jul 26, 2017

coveralls commented Jul 26, 2017

nicoddemus left a comment

coveralls commented Jul 26, 2017

wence- commented Jul 26, 2017

Fix nondeterminism in fixture collection order #2617

Fix nondeterminism in fixture collection order #2617

Conversation

wence- commented Jul 25, 2017 • edited Loading

The-Compiler commented Jul 25, 2017

coveralls commented Jul 25, 2017

nicoddemus commented Jul 25, 2017

RonnyPfannschmidt commented Jul 26, 2017

wence- commented Jul 26, 2017

wence- commented Jul 26, 2017

The-Compiler commented Jul 26, 2017

coveralls commented Jul 26, 2017

coveralls commented Jul 26, 2017

wence- commented Jul 26, 2017

nicoddemus commented Jul 26, 2017

nicoddemus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 26, 2017

nicoddemus left a comment

Choose a reason for hiding this comment

coveralls commented Jul 26, 2017

wence- commented Jul 26, 2017

wence- commented Jul 25, 2017 •

edited

Loading