Skip to content

Generate parametrize IDs for enum/re/class objects. #923

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

Merged
merged 6 commits into from
Aug 8, 2015
Merged

Generate parametrize IDs for enum/re/class objects. #923

merged 6 commits into from
Aug 8, 2015

Conversation

The-Compiler
Copy link
Member

I noticed my tests were generating default IDs for those, so I thought why not make pytest a bit smarter 😉

@The-Compiler The-Compiler added type: enhancement new feature or API change, should be merged into features branch topic: parametrize related to @pytest.mark.parametrize labels Aug 7, 2015
@The-Compiler
Copy link
Member Author

Should I add a # pragma: no coverage for the ImportError branch for enum? It should be tested by the python2 test environments.

@@ -1,6 +1,9 @@
2.8.0.dev (compared to 2.7.X)
-----------------------------

- parametrize now also generates meaningful test IDs for enum, regex and class
objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add something like "(as opposed to class instances)" just to be clear

Copy link
Member

Choose a reason for hiding this comment

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

Also add a "Thanks Florian Bruhin for the PR" for consistency with the other entries. 😉

@RonnyPfannschmidt
Copy link
Member

personally i'd escape the regex pattern and add type: in front of the class name.

can you move type(re.compile('')) to a constant?

@RonnyPfannschmidt
Copy link
Member

oh, and perhaps the module also, it's thinkable that people use different classes with the same module local name as parameter (i have at least one project where i do)

@nicoddemus
Copy link
Member

@RonnyPfannschmidt

oh, and perhaps the module also, it's thinkable that people use different classes with the same module local name as parameter (i have at least one project where i do)

In the same parametrized test? I'm not sure, I feel this is a rare occurrence and it would make the output of all the other cases more verbose without much gain...

@The-Compiler
Copy link
Member Author

personally i'd escape the regex pattern

What do you mean?

>>> print(re.compile(r'foo\bar').pattern)
foo\bar

Would you expect foo\\bar instead? Personally the current behaviour is what I expect - in fact, it's the same as for strings ("foo\\bar" as parameter -> foo\bar as ID).

can you move type(re.compile('')) to a constant?

Sure, will do.

and add type: in front of the class name.

oh, and perhaps the module also, it's thinkable that people use different classes with the same module local name as parameter (i have at least one project where i do)

I don't think those are very useful - I'd prefer to keep the test IDs short. I usually know a test is parametrized with classes (which I usually call klass), and would prefer test_conftype[ClassName] to test_conftype[type: ClassName] or the current test_conftypes[klass23].

So if you don't feel very strong about those two, I'd like to keep it as-is.

@nicoddemus
Copy link
Member

Guys,

This PR brought to my mind a topic which I have thought about before but forgot to bring up (actually I think it was @fogo which thought about this): what do you think about introducing a new hook which can be used by plugins to customize the ids generated by parametrize? This would allow users to generate readable parameter ids even for project-specific objects.

Something along the lines of:

def pytest_make_parametrize_id(config, val):
    """Return a user-friendly string representation of the given ``val`` that will be used 
    by @pytest.mark.parametrize calls. Return None if the hook doesn't know about ``val``.
    """

(I'm not proposing to add this to the PR, just thought I would mention it here; the PR review/merge process should continue regardless of this).

@The-Compiler
Copy link
Member Author

I like the idea! That might make sense for some Qt types for pytest-qt for example.

I wonder however: What's the reason pytest doesn't just use repr() or str() on any object? What's wrong with e.g. a dict being printed that way as well? I'm sure there's a good reason, but I can't see it right now.

@RonnyPfannschmidt
Copy link
Member

some types have inconsistent repr/str per run, dicts for example - the randomized hash-seed makes the problem more apparent

that would create trouble for xdist

@The-Compiler
Copy link
Member Author

I did the suggested style changes.

(disclaimer: I didn't run any tests locally since I'm running out of battery)

@nicoddemus
Copy link
Member

please turn RegexType into the global REGEX_TYPE

@The-Compiler, while you are it, please also move the try/except enum import to the global level. I think it is worth optimizing this and REGEX_TYPE as this function will be called for every parametrized value during collection time.

import enum
except ImportError:
# Only available in Python 3.4+
enum = None
Copy link
Member

Choose a reason for hiding this comment

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

You can put a #pragma: no cover comment on this line to keep code coverage happy here. 😄

Copy link
Member

Choose a reason for hiding this comment

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

also note that there is a backport, so the comment is inaccurate :)

@The-Compiler
Copy link
Member Author

@nicoddemus I did that too, adjusted the comment and added a # pragma: no cover 😄

@nicoddemus
Copy link
Member

Thanks! 😄

Looks 👍 to me! If nobody has anything else to add, I will merge this later!

@pfctdayelise
Copy link
Contributor

@nicoddemus, re your hook idea, ids can be specified as a list of strings or as a callable (#351). I feel like that is probably equivalent to having a hook, not sure though.

@The-Compiler, why doesn't pytest use str/repr on all objects, many dicts would quickly become unwieldy if printed like that, and many user-written str/reprs are actually not that good, for example would not be unique for different objects. Or again, would just be unwieldly. You want something that will fit on a line and preferably not take the whole line (IMO).

@nicoddemus
Copy link
Member

re your hook idea, ids can be specified as a list of strings or as a callable

It's not quite the same thing, as it requires that every parametrized call to pass a function to specify the ids. A hook would be installed once and used automatically for all parametrized calls.

It is of course a matter of convenience. Anyway, I will create a new issue with this and we can move the discussion to there. 😄

@The-Compiler
Copy link
Member Author

You want something that will fit on a line and preferably not take the whole line (IMO).

I agree - but the current handling already allows strings, where this might not be the case:

tests/browser/test_webelem.py::TestWebElementWrapper::test_debug_text[<foo>xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx</foo>-<foo>xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx…] PASSED
tests/utils/test_utils.py::TestCompactText::test_compact_text[foo
bar-foobar] PASSED
tests/utils/test_utils.py::TestCompactText::test_compact_text[  foo  
  bar  -foobar] PASSED
tests/utils/test_utils.py::TestCompactText::test_compact_text[
foo
-foo] PASSED
tests/utils/test_utils.py::TestCompactText::test_eliding[None-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx] PASSED
tests/utils/test_utils.py::TestCompactText::test_eliding[6-foobar-foobar] PASSED
tests/utils/test_utils.py::TestCompactText::test_eliding[5-foobar-foob…] PASSED
tests/utils/test_utils.py::TestCompactText::test_eliding[5-foo
bar-foob…] PASSED
tests/utils/test_utils.py::TestCompactText::test_eliding[7-foo
bar-foobar] PASSED

I wonder if pytest actually should do something like my compact_text function to test IDs (strip and join multiple lines, and an ellipsis if it's too long).

@pfctdayelise
Copy link
Contributor

Keeping in mind that the id should be unique as it forms the test name, I would say messing with string contents for ids is a bad idea. If the user has given distinct input we should try to avoid forcing them to specify ids, better to have boring ids. Yes if you have long strings you will have long IDs, but I don't see that as an argument for extending the badness to dicts and other types. Surely more a good case for using custom ids instead.

@RonnyPfannschmidt
Copy link
Member

basically for all complex datasets, custom id's win flat over printing directly, since meaningfull names

nicoddemus added a commit that referenced this pull request Aug 8, 2015
Generate parametrize IDs for enum/re/class objects.
@nicoddemus nicoddemus merged commit 729b5e9 into pytest-dev:master Aug 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: parametrize related to @pytest.mark.parametrize type: enhancement new feature or API change, should be merged into features branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants