Fix ScopeMismatch with parametrized cases #317
Conversation
Fixtures that are to be injected in conftest.py will be available globally. If we do not include this information in the name of the autogenerated fixtures, we may risk causing a conflict if another test/case/conftest uses parametrization on the same tests.
When conftest is in a package rather than in a mere folder, its name is "fully qualified". Perhaps there would be no need to actually add the scope in this case, but better safe than sorry.
|
@smarie, any chance to get the workflow above approved? This way I can officially make sure there are no conflicts |
|
Sorry @michele-riva , I am so late on my GH duties. Approved and unfortunately... failing :( |
|
Thanks, I am having a look at it. Seems I overlooked the default for the scope kwarg. |
Make sure that the default scope always is "function" so as to avoid issues with pytest <= 6, which 'translates' the scope string kwarg into an index from a list. The list does not contain None.
|
I think I have a fix. Would you mind approving the workflow once again? |
sure. Sorry for this default github behaviour, if I remember correctly once you'll have contributed once then I will not have to approve anymore |
|
No worries! Thanks for the quick reply |
|
Great, looks like that did the trick. Feel free to suggest any change if you don't like the solutions. |
As suggested by @smarie
|
|
||
| @fixture | ||
| @parametrize_with_cases('arg', cases='cases') | ||
| def function_scoped(arg): |
There was a problem hiding this comment.
Same comment as for the class-scoped: could you imagine a way to include this fixture in the test too, and in particular a way to be sure that the scope is correctly taken into account for all 3 ?
|
|
||
| @fixture(scope='class') | ||
| @parametrize_with_cases('arg', cases='cases', scope='class') | ||
| def class_scoped(arg): |
There was a problem hiding this comment.
This fixture does not appear to be used in the test. To be sure that the test works (and not only the pytest initial collection/setup), I would recommend to use this fixture somewhere in the test. In particular, the test does not at all validate that the scope is actually used.
This might require to create a second test, and to wrap both in a class..
|
Overall great contribution @michele-riva , thanks ! In particular kudos for being able to walk your way through the horrible convoluted code of fixture parametrization :D In addition to the comments above, could you please include a changlog entry for your contribution ? |
Sure! Will check back in when I have done all of these |
|
I have just modified the tests as per your suggestion. The check is somewhat indirect, though. I considered using a |
|
Just updated the changelog in 02484e8. From my perspective all is good to go. @smarie, let me know if there's anything else that you'd like changed. See also my #317 (comment) |
|
All good thanks @michele-riva ! |
|
Wonderful, thanks for checking! |
This PR fixes #311.
Tests that show the problem are also included.