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

Test case verbosity #11653

Merged
merged 15 commits into from Feb 24, 2024
Merged

Test case verbosity #11653

merged 15 commits into from Feb 24, 2024

Conversation

plannigan
Copy link
Contributor

Allow for the output of test case execution to be controlled independently from the application verbosity level. verbosity_test_case is the new ini setting to adjust this functionality.

Addresses #11639

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.

Thanks @plannigan for following up with this. 👍

Please take a look at my comments.

One thing I'm not entirely happy is with the "test cases" name, as that IMHO does not convey at all what the flag does. On the other hand I don't have a good suggestion to provide. Would really like some input from other maintainers here (@bluetech @RonnyPfannschmidt @The-Compiler).

changelog/11639.feature.rst Outdated Show resolved Hide resolved
doc/en/how-to/output.rst Outdated Show resolved Hide resolved
changelog/11639.feature.rst Outdated Show resolved Hide resolved
doc/en/reference/reference.rst Outdated Show resolved Hide resolved
doc/en/reference/reference.rst Outdated Show resolved Hide resolved
testing/test_terminal.py Outdated Show resolved Hide resolved
testing/test_terminal.py Outdated Show resolved Hide resolved
testing/test_terminal.py Outdated Show resolved Hide resolved
testing/test_terminal.py Outdated Show resolved Hide resolved

def _fine_grained_verbosity_file_contents() -> str:
long_text = "Lorem ipsum dolor sit amet " * 10
return f"""
Copy link
Member

@nicoddemus nicoddemus Dec 2, 2023

Choose a reason for hiding this comment

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

Not sure we need this complex tests, just a simple parametrized test would allow us to test the purpose of the flag just as well, right?

@pytest.mark.parametrize("i", range(7))
def test_ok(i): pass

The rationale is that we are not testing the details of the output, only that it is more or less verbose according to the option, so the names and the length of the tests docstrings do not plan a role here, and they are already being tested elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started working on this and found that I missed part of the negative verbosity case when executing tests. So I'm going to do some more work on adding more tests for this branch to increase the confidence that we have the correct behavior.

@plannigan
Copy link
Contributor Author

I also don't love verbosity_test_cases. But I think part of the problem is that even just describing what it controls is more complicated than verbosity_assertions. verbosity_test_execution kind of works, but I could see it being misunderstood that it would affect the "Captured stdout" portion of the output. It also makes the --collect-only part less obvious.

I could see an argument that --collect-only is different enough to be it's own thing. But to me, they both feel like "pytest found each test case and is now displaying info as it iterated over them". I was also trying to avoid an over correction that added too many buckets that could be controlled.

@plannigan
Copy link
Contributor Author

I have addressed the initial comments and corrected the bug I identified while re-working the tests. (Thanks for that push. They are now more thorough and easier to follow.)

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.

LGTM!

Will leave it open for a few days to give others a chance to chime in -- perhaps someone will come up with a great suggestion for the option name. 🙏

@bluetech
Copy link
Member

@plannigan In #11387 (comment) you did some great work listing all of the places where verbosity is used. While the assertions verbosity level was a common feature request, I'm not entirely sure about the others (as opposed to using the global verbosity level).

For me, in order to properly evaluate going forward, it will be helpful if you can list all of the categories you're thinking about and how the uses listed in #11387 will categorize.

@nicoddemus
Copy link
Member

I'm not entirely sure about the others (as opposed to using the global verbosity level).

For "test case" specifically, I've seen people wanting to use more verbosity for it, while keeping using less verbosity for assertions (it was internally at work on a specific case), so I think this option has its uses.

it will be helpful if you can list all of the categories you're thinking about and how the uses listed in #11387 will categorize.

It would be helpful indeed to list them in #11387.

@bluetech
Copy link
Member

For "test case" specifically, I've seen people wanting to use more verbosity for it, while keeping using less verbosity for assertions (it was internally at work on a specific case), so I think this option has its uses.

Right, but for this they can bump verbosity and decrease verbosity_assertions now. What I'm curious about is whether there's demand for controlling the remaining cases besides assertions separately.

@plannigan
Copy link
Contributor Author

@bluetech I think it is a good idea to take a step back and consider where this work could go. After reviewing my original list of where verbosity is used and things seem to fall into 3 larger categories:

  • Things in the header
  • Things in the summary (this covers assertions, which has already been merged)
  • The things in between (this PR)

There is one other potential category dealing with traceback filtering and truncating locals. Since that is in the summary, there might be an argument that it should also be grouped with assertions, but at first glance, it feels distinct to me. (Maybe if verbosity_assertions was renamed to verbosity_summary I might think differently.)

As for the "would people use this?" question. I agree that people could achieve it as described. But I feel that having this being the answer is:

  • harder for people to discover
  • a bit clunky (addopts = "-vv" and verbosity_assertions = 0 to do everything in the config file).
  • less explicit - a reader would have to understand the implications of both being used. Going further, the usages might not be next to each other, making it harder to know both are used.

@plannigan
Copy link
Contributor Author

@bluetech @nicoddemus Checking back in as there hasn't been any activity for 2 weeks since my last reply (hopefully you both took some time off for the holidays).

@nicoddemus
Copy link
Member

(hopefully you both took some time off for the holidays).

Indeed this was the case for me.

But the remaining discussion is probably best continued by @bluetech

@plannigan
Copy link
Contributor Author

@bluetech I have updated the branch so that it would be able to be included in the 8.0 release with verbosity_assertions. Do you have any more requests/feedback or can this move forward?

@plannigan
Copy link
Contributor Author

@bluetech It has been two months since I responded to your request for more information. But there has been no response. I updated the branch so that it might be able to be include in 8.0, but that did not happen. There is more work that I'd be interested to contribute, but without any response, I'll likely close the branch and move onto other things.

@bluetech
Copy link
Member

@plannigan Sorry for not responding earlier and letting it linger -- not very nice.

I read your comment but I am not convinced personally. I think it is better to only add configuration knobs when there is demand for it, and my feeling is that there isn't clear demand for this one. I don't mean to say that the feature isn't useful per se, just that IMO it's not useful enough, it's a balancing act.

So I'm -0 on this feature, meaning I won't merge it myself but if any other maintainer feels otherwise that would be perfectly fine by me.

@nicoddemus
Copy link
Member

If @bluetech is -0 on this, I would like to get this merged then. This is something people at work have mentioned in the past so I think it is a nice addition.

I will make a final review tomorrow and merge it then. 👍

@plannigan
Copy link
Contributor Author

Thanks for the responses. I've updated the branch to include the latest of main and reverted that changelog changes I had made to be compatible with the v8 rc. Should be good to go.

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.

Again thanks @plannigan for all the work on this. 👍

@nicoddemus nicoddemus merged commit 84bd31d into pytest-dev:main Feb 24, 2024
24 checks passed
@plannigan plannigan deleted the test-case-verbosity branch February 24, 2024 21:52
flying-sheep pushed a commit to flying-sheep/pytest that referenced this pull request Apr 9, 2024
Allow for the output of test case execution to be controlled independently from the application verbosity level. 

`verbosity_test_case` is the new ini setting to adjust this functionality.

Fix pytest-dev#11639
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.

None yet

3 participants