Skip to content

Indirect session fixture reordering not working as expected #8914

Closed

Description

@nicoddemus

There seems to be a problem with our reordering code that optimizes higher-level fixtures' execution order.

import pytest

@pytest.fixture(scope="session")
def prepare(request):
    print("\nprepare setup with {!r}".format(request.param))

@pytest.mark.parametrize('prepare', ["dina"], indirect=True, scope="session")
def test_1(prepare):
    print("Test1 done\n")

@pytest.mark.parametrize('prepare', ["mor"], indirect=True, scope="session")
def test_2(prepare):
    print("Test2 done\n")

@pytest.mark.parametrize('prepare', ["dina"], indirect=True, scope="session")
def test_3(prepare):
    print("Test3 done\n")

I get this output:

λ pytest .tmp\test_fix_session.py -sq

prepare setup with 'dina'
Test1 done

.
prepare setup with 'mor'
Test2 done

.
prepare setup with 'dina'
Test3 done

.
3 passed in 0.02s

We would expect it to reorder the tests to [test_1, test_3, test_2] so the fixture would execute only twice, instead of 3 times as it is doing now, so definitely looks like a bug (unless I'm missing something).

Thanks @dina809 for the original report (#8328).

Originally posted by @nicoddemus in #8328 (comment)

Activity

  1. added
    topic: fixturesanything involving fixtures directly or indirectly
    type: bugproblem that needs to be addressed
    on Jul 15, 2021
  2. changed the title [-]Indirect session fixture / item reordering issue[/-] [+]Indirect session fixture reordering issue not working as expected[/+] on Jul 15, 2021
  3. changed the title [-]Indirect session fixture reordering issue not working as expected[/-] [+]Indirect session fixture reordering not working as expected[/+] on Jul 15, 2021
  4. Formartha commented on Jul 15, 2021

    @Formartha
  5. symonk commented on Jul 31, 2021

    @symonk
    Member

    I'm gonna have a look into this one next on my hit list, I've read the other thread for all the context and will start debugging soon

  6. added a commit that references this issue on Nov 27, 2021
    38c286d
  7. haxtibal commented on Nov 27, 2021

    @haxtibal
    Contributor

    @nicoddemus This issue hit me as well, hopefully I found the root cause and a possible fix, see this commit

    Reordering test items only took parameter indexes into account. This
    works fine for direct parametrization. But for indirect parametrization,
    the parameter index is always 0, and therefore every item went into the
    same argkey group.

    Now also add the parameter value to the argkey. This is enough
    information to establish different argkey groups and make the grouping
    algorithm work.

    The output from your minimal reproducer is now as expected

    prepare setup with 'dina'
    Test1 done
    Test3 done
    prepare setup with 'mor'
    Test2 done
    

    but the change makes 3 tests fail (test_parameters_without_eq_semantics, test_parameters_without_eq_semantics, test_parametrize_issue634). I've not yet looked at them in detail.

    Could you please give some short feedback? If you think I'm on the right track, I'll try to understand and fix the regression and open a PR here.

  8. nicoddemus commented on Nov 28, 2021

    @nicoddemus
    MemberAuthor

    Hi @haxtibal, that's great! I looked at the code and it makes sense.

    The failing tests are about making sure that pytest works even if the argvalues can't compare with == (raise an error for example).

    I think a solution is to return a wrapper instead of the actual object, something like:

    @attr.s(autoattribs=True, eq=False)
    class SafeEqWrapper:
    
        obj: object
    
        def __eq__(self, other: object) -> Any:
            try:
                res = self.obj == other
                bool(res)
                return res
            except Exception:
                # perhaps return False?
                return id(self.obj) == id(other)

    And then use it like this:

    key: _Key = (argname, param_index, SafeEqWrapper(param_value))

    Feel free to open a PR even if incomplete, we can discuss this over the code. 👍

  9. added 4 commits that reference this issue on Nov 29, 2021
    983713b
    fa26478
    45bcbb4
    5a50852
  10. haxtibal commented on Nov 29, 2021

    @haxtibal
    Contributor

    Thank's for your suggestions @nicoddemus ! We can switch over to #9350 now.

  11. added 4 commits that reference this issue on Dec 9, 2021
    2fdbc3e
    a00c9b1
    ab08f54
    4a314bf
  12. added a commit that references this issue on Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    topic: fixturesanything involving fixtures directly or indirectlytype: bugproblem that needs to be addressed

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions