Fix ScopeMismatch with parametrized cases - #317
Conversation
Sorry, something went wrong.
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, something went wrong.
|
Sorry @michele-riva , I am so late on my GH duties. Approved and unfortunately... failing :( |
Sorry, something went wrong.
|
Thanks, I am having a look at it. Seems I overlooked the default for the scope kwarg. |
Sorry, something went wrong.
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? |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
No worries! Thanks for the quick reply |
Sorry, something went wrong.
|
Great, looks like that did the trick. Feel free to suggest any change if you don't like the solutions. |
Sorry, something went wrong.
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 ?
Sorry, something went wrong.
|
|
||
| @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..
Sorry, something went wrong.
|
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 ? |
Sorry, something went wrong.
Sure! Will check back in when I have done all of these |
Sorry, something went wrong.
|
I have just modified the tests as per your suggestion. The check is somewhat indirect, though. I considered using a request for checking the scopes explicitly, but that was a bit cumbersome. Let me know if you feel like it'd be better to (also?) check explicitly the scope of the FixtureDefs |
Sorry, something went wrong.
|
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) |
Sorry, something went wrong.
|
Wonderful, thanks for checking! |
Sorry, something went wrong.
This PR fixes #311.
Tests that show the problem are also included.