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

Allow hiding a parameter set from test name #13229

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Anton3
Copy link

@Anton3 Anton3 commented Feb 16, 2025

Closes #13228

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Feb 16, 2025
@Anton3 Anton3 marked this pull request as ready for review February 17, 2025 00:18
Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

The edge cases are well handled

We might want to bikeshed on whether to use explicit None vs missing instead of a new singleton

@Anton3
Copy link
Author

Anton3 commented Feb 17, 2025

@RonnyPfannschmidt None is often used in those places to signify "fall back to other ways to get id", so I think the new sigil is justified.

There are currently no public CONSTANTS exported in pytest, so I wondered whether I should use a function instead, but ultimately thought that there are no good reasons not to use a constant there.

Are there any other requirements before merge? If not, then merge when ready :)
Please squash

@RonnyPfannschmidt
Copy link
Member

Thanks for the elaboration 👍

Id like to see the input of some more core members before merge

@Anton3
Copy link
Author

Anton3 commented Feb 17, 2025

@Zac-HD @The-Compiler @jakkdl [pinging random members] Would anyone be willing to also review the PR?

@RonnyPfannschmidt
Copy link
Member

Please avoid eagerly pinging volunteers
Most of us are pretty thin stretched and appreciate the lack of accidental pressure

I understand the desire to finally Lan and previously have been in the same position

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.

Great work!

Left a few comments related mostly to docs.

@@ -89,6 +90,7 @@


__all__ = [
"HIDDEN_PARAM",
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this so the list is kept sorted?

Copy link
Author

Choose a reason for hiding this comment

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

Formatter forces ALL_CAPS names to the top of the list

Comment on lines 1212 to 1213
``pytest.HIDDEN_PARAM`` means to hide the parameter set
from the test name.
Copy link
Member

Choose a reason for hiding this comment

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

This section is shown here: https://pytest--13229.org.readthedocs.build/en/13229/reference/reference.html#pytest.Metafunc.parametrize

Although a bit hidden, perhaps it helps to show the version where this was added, and mentioning the restriction of it being used at most 1 time:

Suggested change
``pytest.HIDDEN_PARAM`` means to hide the parameter set
from the test name.
.. versionadded: 8.4
``pytest.HIDDEN_PARAM`` means to hide the parameter set
from the test name. Can only be used at most 1 time, as
test names need to be unique.

Also HIDDEN_PARAM should appear under the Constants at the top of the page, and then the pytest.HIDDEN_PARAM should be a link to it.

Also it should be mentioned here: https://pytest--13229.org.readthedocs.build/en/13229/reference/reference.html#pytest-param

@@ -0,0 +1 @@
``pytest.HIDDEN_PARAM`` can now be used in ``id`` of ``pytest.param`` or in ``ids`` of ``parametrize``. It hides the parameter set from the test name.
Copy link
Member

Choose a reason for hiding this comment

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

Change pytest.HIDDEN_PARAM to a link to the Constants section in the "API Reference" (see my other comment).

Copy link
Member

Choose a reason for hiding this comment

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

Btw while reviewing #13192 I noticed we will be using NotChecked for a singleton there, should we follow suit here and use HiddenParam too?

Copy link
Author

Choose a reason for hiding this comment

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

I haven't managed to find any mentions of NotChecked in #13192.
I've copied the naming from NOTSET, which is also used around parameter set naming, although not public.
And they both follow PEP8 style for constants.

@Anton3 Anton3 force-pushed the feature/hidden-param branch from 64b14f4 to 72b3441 Compare February 20, 2025 18:24
@Anton3
Copy link
Author

Anton3 commented Feb 20, 2025

@nicoddemus done, please review again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided (automation) changelog entry is part of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide a parameter set from test's name
3 participants