Reusable step functions regression: Specific step doesn't override generic one anymore

[The-Compiler]

After #534, with this conftest.py:

import pytest_bdd as bdd
import pytest

@pytest.fixture
def value():
    return []

@bdd.when(bdd.parsers.parse("I have a {thing} thing"))
def generic(thing, value):
    value.append(thing)

this test_specific.py:

import pytest_bdd as bdd

@bdd.when("I have a specific thing")
def specific(value):
    value.append("42")

@bdd.then(bdd.parsers.parse("The value should be {thing}"))
def check(thing, value):
    assert value == [thing]

bdd.scenarios("specific.feature")

and this specific.feature:

Scenario: Overlapping steps 1
        When I have a specific thing
        Then the value should be 42

Scenario: Overlapping steps 2
        When I have a generic thing
        Then the value should be generic

I would expect that specific takes precedence over generic, i.e., the value for the first test is "42", not "generic". This used to be the case, but isn't anymore after that commit:

    @bdd.then(bdd.parsers.parse("The value should be {thing}"))
    def check(thing, value):
>       assert value == [thing]
E       AssertionError: assert ['specific'] == ['42']
E         At index 0 diff: 'specific' != '42'
E         Use -v to get more diff

Note, however, it does work fine when generic is moved from the conftest.py file into test_specific.py:

[youtux]

Thank you so much for testing pre-release versions! This is extremely helpful. I will look into it.

[youtux]

I'm in early debugging of this, but it seems to be related to lines https://github.com/pytest-dev/pytest-bdd/blob/c0357d534846feaa240a603e2e83efaeb67314fb/pytest_bdd/scenario.py#L41-L43. These lines are supposed to mimic what pytest does to determine the fixture to use.

@The-Compiler do you think it would be possible for pytest to expose a public api to manipulate (or at least access) the fixture data structure? I think we have similar hacks in pytest-factoryboy.

[The-Compiler]

I'm in early debugging of this, but it seems to be related to lines [...]

Thanks for having a look! Let me know if you need help somewhere, though I'm afraid I already have a lot on my plate...

Thank you so much for testing pre-release versions! This is extremely helpful.

In case you're curious: I run a GitHub workflow every night which (via tox) installs all requirements from their development repos (not transitive deps though, to keep things simple).

That helped me find regressions in various projects. I should probably tweet or blog about it so that maybe more projects do this kind of thing...

do you think it would be possible for pytest to expose a public api to manipulate (or at least access) the fixture data structure?

Better exposing fixtures to plugins is something which has been discussed in the past, but from what I can gather, the fixture logic is unfortunately that part of pytest which nobody really dares to touch anymore, because even seemingly innocent changes caused regressions in some corner cases in the past. Somebody would need to step up to clean things up piece for piece, and unfortunately I don't think anybody is interested so far...

[elchupanebrej]

@youtux @The-Compiler I've put some effort to provide such API for pytest-bdd

https://github.com/elchupanebrej/pytest-bdd-ng/blob/default/pytest_bdd/plugin.py

@pytest.fixture
def step_registry() -> StepHandler.Registry:
    """Fixture containing registry of all user-defined steps"""

@pytest.fixture
def step_matcher(pytestconfig) -> StepHandler.Matcher:
    """Fixture containing matcher to help find step definition for selected step of scenario"""
    return StepHandler.Matcher(pytestconfig)  # type: ignore[call-arg]

def pytest_bdd_match_step_definition_to_step(request, feature, scenario, step, previous_step) -> StepHandler.Definition:
    step_registry: StepHandler.Registry = request.getfixturevalue("step_registry")
    step_matcher: StepHandler.Matcher = request.getfixturevalue("step_matcher")

    return step_matcher(feature, scenario, step, previous_step, step_registry)

https://github.com/elchupanebrej/pytest-bdd-ng/blob/cf38255ca37ef417694c6b46523fc98a414e736b/pytest_bdd/steps.py#L210

[elchupanebrej]

My solution doesn't rely on the internal pytest fixtures mechanism, so it has to be pretty stable. It relies only on a cascade of fixture scopes, which, I believe, won't change in the future because it's one of the most fundamental parts of pytest itself. Steps are registered in the closest Step registry, step registries are registered to each other. Step matcher starts to search the corresponding step from the closest registry to the upper one.

[youtux]

@elchupanebrej interesting approach. Does it work fine if you have step definitions defined somewhere and you import them in multiple modules multiple times?

[elchupanebrej]

@youtux I don't think so (because of how registries are placed into module scope), but I'll check and return here. If it doesn't I'll prepare plugin-based solution

[youtux]

I added a test that has many edge cases that should make this bug more visible: https://github.com/pytest-dev/pytest-bdd/pull/544/files#diff-79236ae04de04294d3972ec223e68c0268f0d14ab7bcda5e8ee9e9548661efc5R233.

I run this test against version 6.0.1, and as I expected, it failed. It turned out that this was a bug for a very long time, it's just that with #534 we don't give higher priority to steps without parsers (they are not treated equally), so this bug is way more visible now. I am still not sure how to fix this properly 😞

[elchupanebrej]

@youtux I don't think so (because of how registries are placed into module scope), but I'll check and return here. If it doesn't I'll prepare the plugin-based solution.

I've checked and from my perspective, it's not possible to just import steps without registration to make them work implicitly, and not use "hacky" ways based on internal pytest APIs (It is still possible to use the current pytest-bdd solution, but there should be context resolution issues or add pytest plugin patching on the fly. I don't like both approaches)

[youtux]

Currently with pytest-bdd you can define your steps in a steps/given.py file, and the you can import them in your conftest.py or any test module by doing

from steps.given import *

...

I find this ability important, as it allows to make your step definitions modular.

[elchupanebrej]

I propose another solution to re-use imported steps: https://github.com/elchupanebrej/pytest-bdd-ng/pull/63

Currently with pytest-bdd you can define your steps in a steps/given.py file, and the you can import them in your conftest.py or any test module by doing

from steps.given import *

...

I find this ability important, as it allows to make your step definitions modular.

Usually, it's solved by placing step definitions at according conftest.py. The trade-off between comfortable step imports, overlapping steps hierarchy, re-implementing pytest internals, and monkey patching weights to overlapping steps hierarchy IMO(if there is a possibility to import steps in some way).

Thanks for a good test for step hierarchy, it works perfectly with my solution

[youtux]

To make my example more clear, in my generic conftest.py for a big project, I have the steps defined in hierarchy of packages, like this:

tests/
    conftest.py
    steps/
        user/
            given.py
            when.py
            then.py
        order/
            given.py
            when.py
            then.py
        browser/
            given.py
            when.py
            then.py
        .../
    integration/
        test_feature_a.py
        test_feature_b.py
        ...
    unit/
        ...

and my root level conftest.py looks like this:

# conftest.py
from .steps.users.given import *
from .steps.users.when import *
from .steps.users.then import *

from .steps.order.given import *
from .steps.order.when import *
from .steps.order.then import *

# Steps for interaction with the browser
from .steps.browser.given import *
from .steps.browser.when import *
from .steps.browser.then import *

This is to keep sanity and distinction between common steps for a specific part of the system. I wouldn't want to define all my steps in the root conftest.py, it would be a mess of a file, and it would constantly cause merge conflicts.

[elchupanebrej]

I like your project layout, will try to implement the idea from #310 about auto importing features. If the attempt would be successful there could be a good project layout which would have to be described at documentation as etalon

[youtux]

Hey @The-Compiler, I think I found a fix (test suite is green), but I'd like to ask you to test your project against this fixed version. Could you try commit https://github.com/pytest-dev/pytest-bdd/pull/544/commits/04131286a612b2dd53b2a2d895ecd9233448573c ?

[The-Compiler]

Sorry for the late answer here! I confirm things look good again: https://github.com/qutebrowser/qutebrowser/runs/7552888573?check_suite_focus=true#step:5:33 - thanks for the fix!