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

Extend _pytest.python._idval to return __name__ of functions as well #2976

Merged
merged 6 commits into from Nov 30, 2017

Conversation

Projects
None yet
4 participants
@st--
Contributor

st-- commented Nov 29, 2017

I want to parametrize over factory functions, which I believe is a valid use-case. At the moment, the autogenerated id for the test name is then based on the argument name, plus counter to disambiguate (e.g. feature_factory0, feature_factory1, etc.). With this PR, the default behavior for functions will be the same as for classes: return the __name__. I think this is a more sensible default behaviour.

Semi-pseudocode example:

def factory_IP(T, Kernel): return do_something_with(T, Kernel)
def factory_FF(T, Kernel): return do_something_else(T, Kernel)

pytest.mark.parametrize('feature_factory,kernel_class', [
    (factory_IP, Kernel1),
    (factory_IP, Kernel2),
    (factory_FF, Kernel1),
    (factory_FF, Kernel3),
])
def test_my_model(feature_factory, kernel_class):
    """test code"""

As Kernel1, Kernel2, Kernel3 are classes, currently, the tests would be named as follows:

test_my_model[feature_factory0-Kernel1]
test_my_model[feature_factory1-Kernel2]
test_my_model[feature_factory2-Kernel1]
test_my_model[feature_factory3-Kernel3]

With this PR, the test names would be:

test_my_model[factory_IP-Kernel1]
test_my_model[factory_IP-Kernel2]
test_my_model[factory_FF-Kernel1]
test_my_model[factory_FF-Kernel3]

This allows you to immediately figure out which factory function is being used, and doesn't hide the fact that "feature_factory0" and "feature_factory1" are the same thing.

I hope you'll be happy to merge this :-)

PS: Pytest is great, thanks!!

st-- added some commits Nov 29, 2017

@coveralls

This comment has been minimized.

coveralls commented Nov 29, 2017

Coverage Status

Coverage remained the same at 92.697% when pulling 5085aa2 on st--:master into 88ed1ab on pytest-dev:master.

@coveralls

This comment has been minimized.

coveralls commented Nov 29, 2017

Coverage Status

Coverage remained the same at 92.697% when pulling 5085aa2 on st--:master into 88ed1ab on pytest-dev:master.

@The-Compiler

This comment has been minimized.

Member

The-Compiler commented Nov 29, 2017

This should probably go to the features branch.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 29, 2017

Thanks @st-- for the PR!

Agree with @The-Compiler, changed the target to features. Also @st-- can you please add a test for it to avoid future regressions? Thanks!

@nicoddemus nicoddemus changed the base branch from master to features Nov 29, 2017

@nicoddemus nicoddemus changed the base branch from features to master Nov 29, 2017

@nicoddemus nicoddemus changed the base branch from master to features Nov 29, 2017

@coveralls

This comment has been minimized.

coveralls commented Nov 29, 2017

Coverage Status

Coverage remained the same at 92.697% when pulling fdd4abb on st--:master into 88ed1ab on pytest-dev:features.

@nicoddemus

Please add a test for it to avoid future regressions.

@st--

This comment has been minimized.

Contributor

st-- commented Nov 30, 2017

Hadn't written a test because most of the other cases of _idval didn't have any, either. Happy to add one - I guess in testing/python/metafunc.py ? Should I add one for the isclass() case as well while I'm at it?

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Nov 30, 2017

Happy to add one - I guess in testing/python/metafunc.py ? Should I add one for the isclass() case as well while I'm at it?

Both suggestions sound good, thanks!

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2017

Coverage Status

Coverage remained the same at 92.697% when pulling 1fe2e2c on st--:master into 88ed1ab on pytest-dev:features.

@coveralls

This comment has been minimized.

coveralls commented Nov 30, 2017

Coverage Status

Coverage remained the same at 92.697% when pulling 652936f on st--:master into 88ed1ab on pytest-dev:features.

@st--

This comment has been minimized.

Contributor

st-- commented Nov 30, 2017

Test added, travis passed :)

@nicoddemus

Great, thank you!

@nicoddemus nicoddemus merged commit 2947299 into pytest-dev:features Nov 30, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment