-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
WIP: Truncate objects the same way we truncate other messages #12322
base: main
Are you sure you want to change the base?
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.
Hi @ssbarnea, thanks for the PR!
Btw sorry about the frustration this seems to be causing, I intended to tackle this when I had the time, but free time has eluded me in the past week.
About the solution, while it would work, seems we already consider verbosity when deciding the max size used by saferepr here:
pytest/src/_pytest/assertion/rewrite.py
Lines 428 to 438 in 93dd34e
def _get_maxsize_for_saferepr(config: Optional[Config]) -> Optional[int]: | |
"""Get `maxsize` configuration for saferepr based on the given config object.""" | |
if config is None: | |
verbosity = 0 | |
else: | |
verbosity = config.get_verbosity(Config.VERBOSITY_ASSERTIONS) | |
if verbosity >= 2: | |
return None | |
if verbosity >= 1: | |
return DEFAULT_REPR_MAX_SIZE * 10 | |
return DEFAULT_REPR_MAX_SIZE |
So ideally we should just use that function in order to honor that. Seems to me it could be a matter of just replacing saferepr
by _saferepr
here:
pytest/src/_pytest/assertion/rewrite.py
Line 454 in 93dd34e
obj = saferepr(obj) |
If that works, I think we can even consider this a bug fix, as it seems this should be using _saferepr
all along, being an oversight.
@nicoddemus Apparently that change does not fix it and now we have a separate bug report for this problem. I tried to find the root cause but run out of time for today. I will first add a reproducing test for as we clearly do not want this to regress in the future. |
Thanks @ssbarnea! |
Fixes: #12307
Related: #6682