-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Test case verbosity #11653
Conversation
There was a problem hiding this 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).
testing/test_terminal.py
Outdated
|
||
def _fine_grained_verbosity_file_contents() -> str: | ||
long_text = "Lorem ipsum dolor sit amet " * 10 | ||
return f""" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I also don't love I could see an argument that |
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.) |
There was a problem hiding this 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. 🙏
@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. |
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 would be helpful indeed to list them in #11387. |
Right, but for this they can bump |
@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
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 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:
|
@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). |
Indeed this was the case for me. But the remaining discussion is probably best continued by @bluetech |
@bluetech I have updated the branch so that it would be able to be included in the 8.0 release with |
@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. |
@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. |
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. 👍 |
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. |
There was a problem hiding this 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. 👍
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
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