Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add OuputVerbosity and use it in assertions #11473
Add OuputVerbosity and use it in assertions #11473
Changes from 1 commit
c93bc1e
64f7277
12cb693
5b36f86
cb9729b
ec76cf5
933742a
8e38fc0
cfb1654
4d7ceb8
855f579
d2529ed
c5cc08a
1e91814
b8714de
50c0b93
192b62d
1baca27
21d8111
675584f
9580f4b
b47b203
91bfcab
5d2183d
6ea1fe5
8fdfc1e
e663889
d943866
b91a467
59e39c8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Since we only have one aspect at the moment, it seems premature to write the prose in a general/list format.
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.
My intention is to implement some of the other verbosity types I identified once this PR is merged. A different suggestion was to add a note that only one is currently supported. That seems sufficient to me if "life happens" and it takes me some time to get to adding other types.
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.
Why
OutputVerbosity
and not justVerbosity
? I can't think of a verbosity that is not related to output, so it seems redundant.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'm open tweaking the name. I generally default to more explicit names.
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 think if one passes an invalid
output_type
at the moment, we will get an error (due toself._config.getini
).We probably want to have this classs accessible to plugins and users, via
config.output_verbosity
, so we need to think about its API carefully as we will need to support this officially moving on.Some points:
In tandem with pytest's direction regarding type safety, I think we should use an
Enum
with the output types currently available, something along the lines of:Instead of a simple
output_type: str
parameter/value.The class name is
OutputVerbosity
and the public methods areverbosity
andverbosity_for
, which read a bit redundant:confg.output_verbosity.verbosity
,confg.output_verbosity.get_verbosity_for
. I suggest we use simplyget
, defaulting to theGlobal
verbosity:Which reads well I think:
We need to properly document both
OutputVerbosity
andVerbosityType
in the reference docs (including some usage examples, but possibly we can write those in the class docstring). We also need to exposeVerbosityType
in thepytest
namespace.Given
OutputVerbosity
is public, we should makeOutputVerbosity.addini
private, as it is intended for internal use only.I'm not married to the names above, so suggestions and bikeshedding are welcome.
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.
str
because I was thinking that plugins might want also register types, like any other config setting. Which would mean that it wouldn't be possible to statically define all of the types. However, reflecting on that, I'm not sure that level of extensibility is necessary. It is likely better forpytest
to say "here are N buckets that a plugin can utilize" and that way users of multiple plugins that leverage this functionality will see consistent behavior without needing to enable M settings.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.
Since many things will add these type of settings, I thought it would be helpful to have this function to ensure it is dong consistently. I'm not completely in love with this level of indirection. But it is necessary since config settings are added to the
Parser
before theConfig
object exists.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.
Good idea. 👍
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.
With the move away from
getoption()
, this needed to get a bit more complicated. (In the long term, it would probably be beneficial move the code base to a consistent way to access the verbosity level.)Check warning on line 24 in testing/test_assertion.py
Codecov / codecov/patch
testing/test_assertion.py#L24
Check warning on line 29 in testing/test_assertion.py
Codecov / codecov/patch
testing/test_assertion.py#L29
Check warning on line 32 in testing/test_assertion.py
Codecov / codecov/patch
testing/test_assertion.py#L32