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

pytest 3.3.0 immutable fixture params #2959

Closed
popravich opened this Issue Nov 28, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@popravich

popravich commented Nov 28, 2017

Before v3.3.0 it was possible to mutate fixture parameters, since v3.3.0 params gets "frozen"
(I guess its introduced with this commit 07b2b18)

I was using this feature to setup fixture parameters based on command-line options:

BINS = []

@pytest.fixture(scope='session', params=BINS, ids=...)
def server_bin(request):
    return request.param

@pytest.fixture(scope='session')
def run_server(server_bin):
    pass # start application

def pytest_configure(config):
    BINS[:] = config.getoption('--server')[:]

So my questions:

  • Is this new behavior intended or its a bug?
  • And if its intended what is new way to make dynamic parameters?
@popravich

This comment has been minimized.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Nov 28, 2017

its neither intended, not was it a bug

we simply didn't fall over because users would mutate random aliased objects that would then end up in the internals

the correct way to make fixtures dynamic is something like this:

def pytest_configure(config):
  class DynamicFixturePlugin(object):
     @pytest.fixture(scope='session', params=config.getoption('--servers'), ids=...)
     def server_bin(request):
        return request.param
    config.pluginmanager.register(DynamicFixturePlugin(), 'server-bin-fixture')
@popravich

This comment has been minimized.

popravich commented Nov 28, 2017

Ok, thanks,
I will try it

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 28, 2017

Do we still consider this regression?

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Nov 28, 2017

@nicoddemus it practically is a regression, but its one i'd like to not fix since fixing it makes things worse in the internals again

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 28, 2017

@RonnyPfannschmidt I agree, doesn't seem like something we want to actually fix, at least not in its current form where it was working by accident rather than by design.

@popravich when you can please let us know if the workaround worked for you.

@argaen

This comment has been minimized.

argaen commented Nov 29, 2017

I just found out about this today. Is it worth it to at least add a disclaimer somehow in the CHANGELOG? Our deployment to PROD was skipping some tests without us even noticing so I believe its something worth to comment.

@popravich

This comment has been minimized.

popravich commented Nov 29, 2017

@nicoddemus, the suggested code works with a tiny fix

def pytest_configure(config):
  class DynamicFixturePlugin(object):
     @pytest.fixture(scope='session', params=config.getoption('--servers'), ids=...)
-     def server_bin(request):
+     def server_bin(self, request):
        return request.param
    config.pluginmanager.register(DynamicFixturePlugin(), 'server-bin-fixture')

And I'm +1 to @argaen about some notes in CHANGELOG.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 29, 2017

@argaen and @popravich agree, thanks for the suggestion: #2980

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