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

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

Merged
merged 6 commits into from
Nov 30, 2017

Conversation

st--
Copy link
Contributor

@st-- 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!!

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@The-Compiler
Copy link
Member

This should probably go to the features branch.

@nicoddemus
Copy link
Member

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 November 29, 2017 23:07
@nicoddemus nicoddemus changed the base branch from features to master November 29, 2017 23:07
@nicoddemus nicoddemus changed the base branch from master to features November 29, 2017 23:09
@coveralls
Copy link

Coverage Status

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

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test for it to avoid future regressions.

@st--
Copy link
Contributor Author

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
Copy link
Member

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

@st--
Copy link
Contributor Author

st-- commented Nov 30, 2017

Test added, travis passed :)

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you!

@nicoddemus nicoddemus merged commit 2947299 into pytest-dev:features Nov 30, 2017
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 this pull request may close these issues.

4 participants