Skip to content
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

Resolve param indices using param values, not parameterset index #11257

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

sadra-barikbin
Copy link
Contributor

@sadra-barikbin sadra-barikbin commented Jul 28, 2023

To compute param indices smarter when user passes multiple parameter sets with duplicate values to parametrize in order to have a better reordering experience. Currently we have :

@pytest.fixture(scope='module')
def fixture1(request):
    pass

@pytest.fixture(scope='module')
def fixture2(request):
    pass

@pytest.mark.parametrize("fixture1, fixture2", [("a", 0), ("b", 1), ("a", 2)], indirect=True)
def test(fixture1, fixture2):
    pass

resulting in inefficient ordering i.e.

<Function test[a-0]>
<Function test[b-1]>
<Function test[a-2]>

It's a follow-up to #11220 .

@RonnyPfannschmidt
Copy link
Member

@bluetech after some review of #519 and #2207 i believe it looks fair to consider the ordering more correct

i think we need a crosscheck of how we can observe interactions if people have multiple tests with different sets of parameters and their setup/teardown

@bluetech
Copy link
Member

@sadra-barikbin Can you please rebase this on latest main?

to use 'pytester.runpytest' instead of using testing/python/metafunc.py::TestMetafunc::Metafunc to see if coverage get fixed
…rmining-param-indices-in-metafunc-parametrize
…rmining-param-indices-in-metafunc-parametrize
…rmining-param-indices-in-metafunc-parametrize
@sadra-barikbin sadra-barikbin force-pushed the Improvement-catch-duplicate-values-when-determining-param-indices-in-metafunc-parametrize branch from 054a0ae to 18b7147 Compare December 29, 2023 16:29
…rmining-param-indices-in-metafunc-parametrize
…-duplicate-values-when-determining-param-indices-in-metafunc-parametrize' into Improvement-catch-duplicate-values-when-determining-param-indices-in-metafunc-parametrize
@RonnyPfannschmidt
Copy link
Member

Apologies but I'll be delayed in reviewing this, I'm still down with a cold

It would have been nice to land this in 8.x

@sadra-barikbin
Copy link
Contributor Author

Apologies but I'll be delayed in reviewing this, I'm still down with a cold

It would have been nice to land this in 8.x

It's alright. I hope you recover soon.

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we have to document this better

from my pov we assign parameters that are used multiple times in a expanded/multiplied parameter set the index of the firs occurrence

this can be seen as approximation of a "multi dimensional" index from filtered cross-product

src/_pytest/python.py Outdated Show resolved Hide resolved
@sadra-barikbin
Copy link
Contributor Author

@bluetech , could you please review the PR too?

@cyw233
Copy link

cyw233 commented May 13, 2024

Hi @sadra-barikbin, do we have any plan to merge this PR, please? cc. @RonnyPfannschmidt

@obestwalter
Copy link
Member

obestwalter commented Jun 20, 2024

Just resolved a minor conflict. Let's see if we can get this merged as it is already approved.

@obestwalter
Copy link
Member

Our shiny new bot also enforces changelog entries now, so this needs to be added before it can be merged - could you please add that @sadra-barikbin?

@webknjaz
Copy link
Member

@sadra-barikbin there's another merge conflict to be resolved in addition to the need of having a changelog fragment.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Oct 11, 2024
@sadra-barikbin
Copy link
Contributor Author

I resolved the conflict but this PR introduces is a breaking change with regard to how indices are assigned to the direct params of the test functions (hence one of the tests fail). Currently, when whole parametrizations of a function take place ,the indices of a direct param is assigned to its occurrences sequentially all at once:

pytest/src/_pytest/python.py

Lines 1452 to 1456 in f373974

def _recompute_direct_params_indices(self) -> None:
for argname, param_type in self._params_directness.items():
if param_type == "direct":
for i, callspec in enumerate(self._calls):
callspec.indices[argname] = i

To be able to merge this PR, I should bring the resolve_values_indices_in_parametersets logic in the method above which introduces change to the whole reordering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants