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

ScopeMismatch with parametrized cases #311

Closed
michele-riva opened this issue Sep 13, 2023 · 7 comments · Fixed by #317
Closed

ScopeMismatch with parametrized cases #311

michele-riva opened this issue Sep 13, 2023 · 7 comments · Fixed by #317

Comments

@michele-riva
Copy link
Contributor

Hi, here's the last one. A bit hairier.

Example to reproduce:

cases.py

from pytest_cases import parametrize

@parametrize(arg=(1,))
def case_parametrized(arg):
    return arg

conftest.py

from pytest_cases import fixture, parametrize_with_cases

@fixture(scope='session')
@parametrize_with_cases('arg', cases='cases', scope='session')
def scope_mismatch(arg):
    return arg

test_scope_mismatch.py

from pytest_cases import fixture, parametrize_with_cases

@fixture
@parametrize_with_cases('arg', cases='cases')
def function_scoped(arg):
    return arg

def test_scope_mismatch(scope_mismatch):
    assert scope_mismatch == 1

Gives:

____________________________________________ ERROR at setup of test_scope_mismatch[parametrized-arg=1] _______________________________
ScopeMismatch: You tried to access the function scoped fixture parametrized with a session scoped request object, involved factories:
conftest.py:6:  def scope_mismatch(scope_mismatch_arg, request)
<makefun-gen-5>:1:  def scope_mismatch_arg(parametrized, request)
cases.py:6:  def case_parametrized(request)

Notice that this does not happen if the two fixtures are defined in the same module (either conftest.py or test_scope_mismatch.py). Also, this may be obscured by #309 or #310 if using AUTO cases.

It looks like the core of the issue is that the scope keyword argument is not passed to the .create calls here

def _create_params_alt(fh, test_func, union_name, from_i, to_i, hook): # noqa
""" Routine that will be used to create a parameter fixture for argvalues between prev_i and i"""
# is this about a single value or several values ?
if to_i == from_i + 1:
i = from_i
del from_i
# If an explicit list of ids was provided, slice it. Otherwise use the provided callable
if ids is not None:
_id = ids[i] if explicit_ids_to_use else ids(argvalues[i])
else:
_id = None
return SingleParamAlternative.create(new_fixture_host=fh, test_func=test_func,
param_union_name=union_name, argnames=argnames, i=i,
argvalue=marked_argvalues[i], id=_id,
hook=hook, debug=debug)
else:
# If an explicit list of ids was provided, slice it. Otherwise the provided callable will be used later
_ids = ids[from_i:to_i] if explicit_ids_to_use else ids
return MultiParamAlternative.create(new_fixture_host=fh, test_func=test_func,
param_union_name=union_name, argnames=argnames, from_i=from_i,
to_i=to_i, argvalues=marked_argvalues[from_i:to_i], ids=_ids,
hook=hook, debug=debug)

In principle I can draft a PR.

@michele-riva
Copy link
Contributor Author

Sorry, this seems a bit more complicated than just passing on scope as I suggested above. It appears to be an issue of name conflict for case-parametrized fixtures that are defined within conftest.py. The name-conflict resolution here

# not yet known there. Create a new symbol in the target host :
# we need a "free" fixture name, and a "free" symbol name
existing_fixture_names = []
# -- fixtures in target module or class should not be overridden
existing_fixture_names += list_all_fixtures_in(target_host, recurse_to_module=False)
# -- are there fixtures in source module or class ? should not be overridden too
if not in_same_module(target_host, true_case_func_host):
fixtures_in_cases_module = list_all_fixtures_in(true_case_func_host, recurse_to_module=False)
if len(fixtures_in_cases_module) > 0:
# EXPERIMENTAL we can try to import the fixtures into current module
if import_fixtures:
from_module = get_host_module(true_case_func_host)
if from_module not in imported_fixtures_list:
for f in list_all_fixtures_in(true_case_func_host, recurse_to_module=False, return_names=False):
f_name = get_fixture_name(f)
if (f_name in existing_fixture_names) or (f.__name__ in existing_fixture_names):
raise ValueError("Cannot import fixture %r from %r as it would override an existing symbol "
"in %r. Please set `@parametrize_with_cases(import_fixtures=False)`"
"" % (f, from_module, target_host))
target_host_module = target_host if not target_in_class else get_host_module(target_host)
setattr(target_host_module, f.__name__, f)
imported_fixtures_list.append(from_module)
# Fix the problem with "case_foo(foo)" leading to the generated fixture having the same name
existing_fixture_names += fixtures_in_cases_module
def name_changer(name, i):
return name + '_' * i
# start with name = case_id and find a name that does not exist
fix_name = check_name_available(target_host, extra_forbidden_names=existing_fixture_names, name=case_id,
if_name_exists=CHANGE, name_changer=name_changer)

does not actually fix the conflict because

  • we're injecting the fixture in conftest itself;
  • fixtures defined in conftest.py are available for all tests.

A solution would be to prepend "conftest_" to the case_id (either above, or in fixture__creation.check_name_available), but this only partly solves the issue. In fact, assume the following test-tree structure:

tests/
    conftest.py
    cases.py
    test_deeper/
        conftest.py
        test_scope_mismatch.py

with all the modules identical to my previous example but, test_deeper/conftest.py

from pytest_cases import fixture, parametrize_with_cases

@fixture(scope='class')
@parametrize_with_cases('arg', cases='cases', scope='class')
def scope_mismatch_deeper(arg):
    return arg

While scope_mismatch_deeper is being parametrized, target_host.__name__ == 'conftest' (and not 'test_deeper.conftest') because test_deeper/conftest.py is currently being imported. This would simply duplicate the 'conftest_'+case_id name, leading to another scope mismatch.

The ideal solution would be to have a list_all_fixtures_in_conftests function to be added to the existing_fixture_names. However, I know too little of pytest & pytest-cases to suggest an implementation.

@michele-riva
Copy link
Contributor Author

Sorry for the spam. Perhaps a simpler solution than a list_all_fixtures_in_conftests would be to include also the scope in the base name to be checked (only if the fixture is injected in a conftest.py).

E.g.,

if target_host.__name__ == 'conftest':
    case_id = 'conftest_' + case_id + '_with_scope_' + scope

This has the disadvantage of making names longer. If this sounds acceptable, I can also include it in the PR.

michele-riva added a commit to michele-riva/python-pytest-cases that referenced this issue Sep 13, 2023
@smarie
Copy link
Owner

smarie commented Oct 11, 2023

Thanks @michele-riva for reporting this issue and working on a solution !
I am sorry that I still not found the bandwidth to review this, but yes please feel free to open a PR in whatever state it is (just prefix it [WIP] or tag it as draft while it is not ready for review).

This is definitely one of the nasty aspects of pytest-cases: it generates a lot of fixtures behind the scenes.

Does your issue also happen when the cases are in the same file than the test file, and cases are just imported with cases='.' ? If so I recommend to create the test as such, it is simpler.

@michele-riva
Copy link
Contributor Author

I am sorry that I still not found the bandwidth to review this, but yes please feel free to open a PR in whatever state it is (just prefix it [WIP] or tag it as draft while it is not ready for review).

No worries, I'm also pretty toasted. I have some code in a fork, and I'll prepare a PR today.

Does your issue also happen when the cases are in the same file than the test file, and cases are just imported with cases='.' ? If so I recommend to create the test as such, it is simpler.

Good question. I'd have to try it out. I'll update the tests if this is the case.

@michele-riva
Copy link
Contributor Author

Does your issue also happen when the cases are in the same file than the test file, and cases are just imported with cases='.' ? If so I recommend to create the test as such, it is simpler.

Looks like the issue does not show up for cases defined in the same file. I'll have to go for the more complicated structure.

@michele-riva
Copy link
Contributor Author

I've just opened #317, which should fix this. Happy to discuss edits if needed.

@smarie
Copy link
Owner

smarie commented Nov 8, 2023

Looks like the issue does not show up for cases defined in the same file. I'll have to go for the more complicated structure.

Ok, thanks for checking! This might explain why this issue has not been reported yet.

smarie added a commit that referenced this issue Nov 10, 2023
* tests: add test for issue #311

* fix: propagate scope in ...ParamAlternative

* fix: mangle fixture name for conftest

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.

* fix: avoid conflict also if __init__.py exists

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.

* Do not pass on None as scope

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.

* More explicit None scope replacement

As suggested by @smarie

* Add note and example for conftest qualname

* Test correctness of scopes

As suggested by @smarie

* Add changelog for #317

* Update docs/changelog.md

---------

Co-authored-by: Sylvain Marié <sylvain.marie@schneider-electric.com>
@smarie smarie closed this as completed Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants