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.

Sign up for GitHub

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

Jump to bottom

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

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

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

wants to merge 19 commits into from

Conversation

sadra-barikbin
Copy link
Contributor

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

To compute param indices smarter to have a better reordering experience. Currently we have :

@pytest.mark.parametrize("arg2", [3, 4])
@pytest.mark.parametrize("arg1", [0, 1, 2], scope='module')
def test1(arg1, arg2):
    pass

def test2():
    pass

@pytest.mark.parametrize("arg1", [0, 1, 2], scope='module')
def test3(arg1):
    pass

resulting in an improper ordering i.e.

<Function test1[0-3]>
<Function test3[0]>
<Function test1[0-4]>
<Function test3[1]>
<Function test1[1-3]>
<Function test3[2]>
<Function test1[1-4]>
<Function test1[2-3]>
<Function test1[2-4]>
<Function test2>

Or 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]>

This PR fixes these.
It's a follow-up to #11220 .

@sadra-barikbin sadra-barikbin mentioned this pull request Jul 22, 2023
Open
@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

@sadra-barikbin sadra-barikbin mentioned this pull request Aug 1, 2023
Merged
@bluetech
Copy link
Member

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

@sadra-barikbin
Change a test
752d23d 
to use 'pytester.runpytest' instead of using testing/python/metafunc.py::TestMetafunc::Metafunc to see if coverage get fixed
@sadra-barikbin
Merge branch 'main' into Improvement-catch-duplicate-values-when-dete…
d33f65d 
…rmining-param-indices-in-metafunc-parametrize
@sadra-barikbin
Merge branch 'main' into Improvement-catch-duplicate-values-when-dete…
af390f5 
…rmining-param-indices-in-metafunc-parametrize
@sadra-barikbin
Merge branch 'main' into Improvement-catch-duplicate-values-when-dete…
82a4e0b 
…rmining-param-indices-in-metafunc-parametrize
@sadra-barikbin
Remove a test
d5bf27c
@sadra-barikbin
Revert a tiny change
18b7147
@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
@sadra-barikbin
Merge branch 'main' into Improvement-catch-duplicate-values-when-dete…
2f33561 
…rmining-param-indices-in-metafunc-parametrize
@sadra-barikbin
Remove a redundant test
49322ce
@sadra-barikbin
Merge remote-tracking branch 'refs/remotes/upstream/Improvement-catch…
ed6c38c 
…-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.

RonnyPfannschmidt
RonnyPfannschmidt approved these changes Jan 5, 2024
View reviewed changes
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?

@RonnyPfannschmidt RonnyPfannschmidt mentioned this pull request Mar 22, 2024
Open
@sadra-barikbin sadra-barikbin mentioned this pull request Mar 23, 2024
Open
@vperumal vperumal mentioned this pull request May 8, 2024
Closed
@cyw233
Copy link

cyw233 commented May 13, 2024

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

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

@RonnyPfannschmidt RonnyPfannschmidt RonnyPfannschmidt approved these changes

@bluetech bluetech Awaiting requested review from bluetech

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sadra-barikbin @RonnyPfannschmidt @bluetech @cyw233