pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
12.01k stars 2.67k forks source link

Remove parametrize decorators (and perhaps docstrings) from tracebacks #8484

Open The-Compiler opened 3 years ago

The-Compiler commented 3 years ago

Consider a test like this:

@pytest.mark.parametrize('text1, text2, equal', [
    # schemes
    ("http://en.google.com/blah/*/foo",
     "https://en.google.com/blah/*/foo",
     False),
    ("https://en.google.com/blah/*/foo",
     "https://en.google.com/blah/*/foo",
     True),
    ("https://en.google.com/blah/*/foo",
     "ftp://en.google.com/blah/*/foo",
     False),

    # subdomains
    ("https://en.google.com/blah/*/foo",
     "https://fr.google.com/blah/*/foo",
     False),
    ("https://www.google.com/blah/*/foo",
     "https://*.google.com/blah/*/foo",
     False),
    ("https://*.google.com/blah/*/foo",
     "https://*.google.com/blah/*/foo",
     True),

    # domains
    ("http://en.example.com/blah/*/foo",
     "http://en.google.com/blah/*/foo",
     False),

    # ports
    ("http://en.google.com:8000/blah/*/foo",
     "http://en.google.com/blah/*/foo",
     False),
    ("http://fr.google.com:8000/blah/*/foo",
     "http://fr.google.com:8000/blah/*/foo",
     True),
    ("http://en.google.com:8000/blah/*/foo",
     "http://en.google.com:8080/blah/*/foo",
     False),

    # paths
    ("http://en.google.com/blah/*/foo",
     "http://en.google.com/blah/*",
     False),
    ("http://en.google.com/*",
     "http://en.google.com/",
     False),
    ("http://en.google.com/*",
     "http://en.google.comm/*",
     True),

    # all_urls
    ("<all_urls>",
     "<all_urls>",
     True),
    ("<all_urls>",
     "http://*/*",
     False)
])
def test_equal(text1, text2, equal):
    pat1 = urlmatch.UrlPattern(text1)
    pat2 = urlmatch.UrlPattern(text2)

    assert (pat1 == pat2) == equal
    assert (hash(pat1) == hash(pat2)) == equal

If a parametrized value fails, we get:

_______________________________________________________________________________________ test_equal[http://en.google.com/*-http://en.google.comm/*-True] _______________________________________________________________________________________

text1 = 'http://en.google.com/*', text2 = 'http://en.google.comm/*', equal = True

    @pytest.mark.parametrize('text1, text2, equal', [
        # schemes
        ("http://en.google.com/blah/*/foo",
         "https://en.google.com/blah/*/foo",
         False),
        ("https://en.google.com/blah/*/foo",
         "https://en.google.com/blah/*/foo",
         True),
        ("https://en.google.com/blah/*/foo",
         "ftp://en.google.com/blah/*/foo",
         False),

        # subdomains
        ("https://en.google.com/blah/*/foo",
         "https://fr.google.com/blah/*/foo",
         False),
        ("https://www.google.com/blah/*/foo",
         "https://*.google.com/blah/*/foo",
         False),
        ("https://*.google.com/blah/*/foo",
         "https://*.google.com/blah/*/foo",
         True),

        # domains
        ("http://en.example.com/blah/*/foo",
         "http://en.google.com/blah/*/foo",
         False),

        # ports
        ("http://en.google.com:8000/blah/*/foo",
         "http://en.google.com/blah/*/foo",
         False),
        ("http://fr.google.com:8000/blah/*/foo",
         "http://fr.google.com:8000/blah/*/foo",
         True),
        ("http://en.google.com:8000/blah/*/foo",
         "http://en.google.com:8080/blah/*/foo",
         False),

        # paths
        ("http://en.google.com/blah/*/foo",
         "http://en.google.com/blah/*",
         False),
        ("http://en.google.com/*",
         "http://en.google.com/",
         False),
        ("http://en.google.com/*",
         "http://en.google.comm/*",
         True),

        # all_urls
        ("<all_urls>",
         "<all_urls>",
         True),
        ("<all_urls>",
         "http://*/*",
         False)
    ])
    def test_equal(text1, text2, equal):
        pat1 = urlmatch.UrlPattern(text1)
        pat2 = urlmatch.UrlPattern(text2)

>       assert (pat1 == pat2) == equal
E       AssertionError: assert (equals failed
E         qutebrowser.utils.urlmatch.UrlPattern(pattern='http://en.google.com/*')   qutebrowser.utils.urlmatch.UrlPattern(pattern='http://en.google.comm/*') ) == True

tests/unit/utils/test_urlmatch.py:690: AssertionError

However, the whole parametrize decorator really is irrelevant - it makes the output very long, contains a lot of values which actually aren't the one the test did run with, and hides the very relevant text1 = 'http://en.google.com/*', text2 = 'http://en.google.comm/*', equal = True at the top.

Ideally, we'd instead have something like:

_______________________________________________________________________________________ test_equal[http://en.google.com/*-http://en.google.comm/*-True] _______________________________________________________________________________________

text1 = 'http://en.google.com/*', text2 = 'http://en.google.comm/*', equal = True

    @pytest.mark.parametrize('text1, text2, equal', [...])
    def test_equal(text1, text2, equal):
        pat1 = urlmatch.UrlPattern(text1)
        pat2 = urlmatch.UrlPattern(text2)

>       assert (pat1 == pat2) == equal
E       AssertionError: assert (equals failed
E         qutebrowser.utils.urlmatch.UrlPattern(pattern='http://en.google.com/*')   qutebrowser.utils.urlmatch.UrlPattern(pattern='http://en.google.comm/*') ) == True

tests/unit/utils/test_urlmatch.py:690: AssertionError

or if that's too hard to do (didn't look into the code so far), perhaps don't show decorators at all:

text1 = 'http://en.google.com/*', text2 = 'http://en.google.comm/*', equal = True

    def test_equal(text1, text2, equal):
        pat1 = urlmatch.UrlPattern(text1)
        pat2 = urlmatch.UrlPattern(text2)

>       assert (pat1 == pat2) == equal
E       AssertionError: assert (equals failed
E         qutebrowser.utils.urlmatch.UrlPattern(pattern='http://en.google.com/*')   qutebrowser.utils.urlmatch.UrlPattern(pattern='http://en.google.comm/*') ) == True

tests/unit/utils/test_urlmatch.py:690: AssertionError

which feels like much less noise.

This came up in the recent pytest chatter talking to @asottile and @RonnyPfannschmidt (and others, but I don't know your GitHub nicks, sorry!).

IIRC @asottile also mentioned a similar annoyance with long docstrings, which I don't have an example for off-hand. Not sure if it makes sense to complete redact docstrings (after all, they could be useful!), but maybe we can do so after a couple of lines?

nicoddemus commented 3 years ago

That's indeed an interesting idea!

perhaps don't show decorators at all

I don't think that's a good idea: at work we have decorators which are important to understand a failure, (a test should have been skipped in that setup but wasn't, marked as slow, and so on). Removing that from the failure would make the failure harder to diagnose.

I completely agree with collapsing pytest.mark.parametrize like in your suggestion: @pytest.mark.parametrize('text1, text2, equal', [...]).

sommersoft commented 2 years ago

I've been poking at this a little, and have a working solution. I have a couple questions before preparing to push a PR.

  1. So far, the scope in this issue seems limited to @pytest.mark.parametrize. Is there any desire to expand to other built-in decorators, or keep it limited and accept the following:

    
    text1 = '<all_urls>', text2 = 'http://*/*', equal = False
    
    @pytest.mark.parametrize("text1, text2, equal", [...])
    @pytest.mark.skipif(
        "7" not in pytest.__version__,
        reason="Testing for long skipif that gets split into multiline.....",
    )
    def test_equal(text1, text2, equal):
    >       assert text1 == text2
    E       AssertionError: assert '<all_urls>' == 'http://*/*'
    E         - http://*/*
    E         + <all_urls>

param/test_params.py:27: AssertionError



2. What is a sensible maximum for docstring lines? My current solution is using `3` lines max.
EpicWink commented 2 years ago

What is a sensible maximum for docstring lines? My current solution is using 3 lines max.

I think docstring collapse should be a separate PR and command-line flag. In answer, I would say to just keep the summary line (not necessarily the first line, but stop at the first black line).


I would say to limit the scope to just parametrize: other decorators can be implemented in a separate PR if there's interest