Implement CLI options to set truncation thresholds #12766

zhukoff-pavel · 2024-09-02T16:55:38Z

Example implementation of truncation limit setting with CLI flags

for more information, see https://pre-commit.ci

nicoddemus · 2024-09-08T23:56:34Z

Thanks @zhukoff-pavel, we appreciate the PR!

I would like to see what other maintainers think of this feature, overall I'm +1 on it, the only downside I see is the cognitive load of extra options... but perhaps others have different opinions here.

I also wonder if this is better implemented as an ini option instead... seems like it would be more common to set this in the config file in test suites where this feature is often wanted (and of course it is always pass this using the -o option in the command line).

changelog/12765.feature.rst

zhukoff-pavel · 2024-09-09T08:29:30Z

@nicoddemus, thank you for your reply!

I also wonder if this is better implemented as an ini option instead... seems like it would be more common to set this in the config file in test suites where this feature is often wanted (and of course it is always pass this using the -o option in the command line).

Maybe it is better to implement an .ini option and CLI flag to e.g. temporarily override limits in a singular launch?

Is it possible to implement such behaviour with current option reading machinery (with some combination of addoption and addini methods)?

nicoddemus · 2024-09-09T11:57:57Z

Maybe it is better to implement an .ini option and CLI flag to e.g. temporarily override limits in a singular launch?

Your suggestion makes sense, however we have for awhile moved away from this because:

It involves a lot of code duplication
We have mechanisms today where users can use them interchangeably (use addopts to hard-code command-line options in the ini file, and use -o to pass config options in the command line).

For this reason nowadays we prefer to have either a command-line option (for features where users are likely to change between pytest invocations) or an ini option (where users are most likely to set once per repository).

Is it possible to implement such behaviour with current option reading machinery (with some combination of addoption and addini methods)?

Unfortunately no, it requires duplicating the code.

zhukoff-pavel · 2024-09-09T12:38:32Z

I see.
I can then rewrite this PR to implement .ini option instead, as it definitely looks more like a one-time config setting rather than per launch.

Thank you for a quick reply!

nicoddemus · 2024-09-09T12:47:33Z

I can then rewrite this PR to implement .ini option instead

Thanks! I suggest to wait a bit in case some other maintainer has comments or reservations about the feature though.

nicoddemus · 2024-09-13T11:01:23Z

Been awhile and nobody seems to be against the idea, so feel free to go ahead.

for more information, see https://pre-commit.ci

nicoddemus

Thanks @zhukoff-pavel!

Please take a look at my comments.

changelog/12765.feature.rst

doc/en/how-to/output.rst

src/_pytest/assertion/truncate.py

testing/test_assertion.py

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

for more information, see https://pre-commit.ci

zhukoff-pavel · 2024-09-16T21:16:20Z

@nicoddemus , thank you for a thorough review!

Please have a look at the changes.

I implemented some logic regarding value of 0 being set in one mode or both at a time.

nicoddemus · 2024-09-16T22:52:51Z

src/_pytest/assertion/truncate.py

    return explanation


 def _should_truncate_item(item: Item) -> bool:
    """Whether or not this test item is eligible for truncation."""
    verbose = item.config.get_verbosity(Config.VERBOSITY_ASSERTIONS)
-    return verbose < 2 and not util.running_on_ci()
+


I don't quite like how we are dealing with truncation_limit_lines and truncation_limit_chars in two places (even if they are next to each other)... feels error prone. 🤔

I think we should change _should_truncate_item to return the max_chars and max_lines to use, and if it returns None means "do no truncate"... if so we should also rename it, perhaps get_truncation_params? This way only get_truncation_params looks up at the options.

What do you think?

Btw this are all internal functions, so we are free to refactor them.

Yes, I also did not like it when I implemented this, sounds good to refactor this place :)

Will think, what I can do here

However, I do think about return values here. None sounds great, but will it be clear that it means "No truncation" instead of using default values?

None sounds great, but will it be clear that it means "No truncation" instead of using default values?

You got a point, for a more visible API probably a new type would be best, but given this is internal and used in a single place, I guess using None is fine (as long as if documented of course).

Or perhaps it just makes sense to return (0, 0) when it should not truncate at all? Whatever makes the logic handling in truncate_if_required simpler.

nicoddemus · 2024-09-16T22:54:04Z

doc/en/how-to/output.rst

@@ -549,6 +549,24 @@ captured output:
    By default, parametrized variants of skipped tests are grouped together if
    they share the same skip reason. You can use ``--no-fold-skipped`` to print each skipped test separately.

+
+Modifying truncation limits


Let's add a link target so we can refer to it from the CHANGELOG:

Suggested change

Modifying truncation limits

.. _truncation-params:

Modifying truncation limits

nicoddemus · 2024-09-16T22:54:25Z

changelog/12765.feature.rst

@@ -0,0 +1 @@
+Thresholds to trigger snippet truncation can now be set with :confval:`truncation_limit_lines` and :confval:`truncation_limit_chars`.


Suggested change

Thresholds to trigger snippet truncation can now be set with :confval:`truncation_limit_lines` and :confval:`truncation_limit_chars`.

Thresholds to trigger snippet truncation can now be set with :confval:`truncation_limit_lines` and :confval:`truncation_limit_chars`.

See :ref:`truncation-params` for more information.

nicoddemus · 2024-09-16T22:55:10Z

doc/en/how-to/output.rst

+
+Modifying truncation limits
+--------------------------------------------------
+


Suggested change

.. versionadded: 8.4

zhukoff-pavel added 3 commits September 2, 2024 19:29

feat: implement customization of truncation limits

fc06bae

create changelog

42e4e9c

append authors

85fe194

psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Sep 2, 2024

pre-commit-ci bot and others added 6 commits September 2, 2024 16:55

[pre-commit.ci] auto fixes from pre-commit.com hooks

24b075b

for more information, see https://pre-commit.ci

fix changelog

550b725

add test

ea7d90b

fix no_fnmatch_line invocations

fff0e7b

escape newlines

3fd50ad

Change order

868c26e

zhukoff-pavel marked this pull request as ready for review September 2, 2024 18:48

zhukoff-pavel added 2 commits September 3, 2024 10:31

Merge branch 'main' into impl-12765

5526cba

Merge branch 'main' into impl-12765

856bb8f

nicoddemus reviewed Sep 9, 2024

View reviewed changes

changelog/12765.feature.rst Outdated Show resolved Hide resolved

Rewrite to .ini parameters instead of CLI

e1edf2c

zhukoff-pavel marked this pull request as draft September 16, 2024 16:39

[pre-commit.ci] auto fixes from pre-commit.com hooks

b4b5fb1

for more information, see https://pre-commit.ci

zhukoff-pavel marked this pull request as ready for review September 16, 2024 16:51

Merge branch 'main' into impl-12765

23ea5dd

nicoddemus requested changes Sep 16, 2024

View reviewed changes

changelog/12765.feature.rst Outdated Show resolved Hide resolved

doc/en/how-to/output.rst Outdated Show resolved Hide resolved

src/_pytest/assertion/truncate.py Show resolved Hide resolved

testing/test_assertion.py Outdated Show resolved Hide resolved

zhukoff-pavel and others added 4 commits September 16, 2024 21:11

Convert CLI flags to confvals in changelog

d123ac3

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

Clarify truncation usage order in doc/en/how-to/output.rst

98938ce

Co-authored-by: Bruno Oliveira <bruno@soliv.dev>

Handle parameters being set to zero

ae3cd2f

[pre-commit.ci] auto fixes from pre-commit.com hooks

b2287af

for more information, see https://pre-commit.ci

Fix typo in test case description

eee9e70

zhukoff-pavel marked this pull request as draft September 16, 2024 20:11

Fix failing tests

7027624

zhukoff-pavel marked this pull request as ready for review September 16, 2024 21:14

zhukoff-pavel requested a review from nicoddemus September 16, 2024 21:16

nicoddemus reviewed Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement CLI options to set truncation thresholds #12766

Implement CLI options to set truncation thresholds #12766

zhukoff-pavel commented Sep 2, 2024 •

edited

Loading

nicoddemus commented Sep 8, 2024

zhukoff-pavel commented Sep 9, 2024

nicoddemus commented Sep 9, 2024 •

edited

Loading

zhukoff-pavel commented Sep 9, 2024 •

edited

Loading

nicoddemus commented Sep 9, 2024

nicoddemus commented Sep 13, 2024

nicoddemus left a comment

zhukoff-pavel commented Sep 16, 2024

nicoddemus Sep 16, 2024

zhukoff-pavel Sep 17, 2024

nicoddemus Sep 17, 2024

nicoddemus Sep 17, 2024

nicoddemus Sep 16, 2024

nicoddemus Sep 16, 2024

nicoddemus Sep 16, 2024

	Modifying truncation limits
	.. _truncation-params:
	Modifying truncation limits

		@@ -0,0 +1 @@
		Thresholds to trigger snippet truncation can now be set with :confval:`truncation_limit_lines` and :confval:`truncation_limit_chars`.


		Modifying truncation limits
		--------------------------------------------------

Implement CLI options to set truncation thresholds #12766

Are you sure you want to change the base?

Implement CLI options to set truncation thresholds #12766

Conversation

zhukoff-pavel commented Sep 2, 2024 • edited Loading

nicoddemus commented Sep 8, 2024

zhukoff-pavel commented Sep 9, 2024

nicoddemus commented Sep 9, 2024 • edited Loading

zhukoff-pavel commented Sep 9, 2024 • edited Loading

nicoddemus commented Sep 9, 2024

nicoddemus commented Sep 13, 2024

nicoddemus left a comment

Choose a reason for hiding this comment

zhukoff-pavel commented Sep 16, 2024

nicoddemus Sep 16, 2024

Choose a reason for hiding this comment

zhukoff-pavel Sep 17, 2024

Choose a reason for hiding this comment

nicoddemus Sep 17, 2024

Choose a reason for hiding this comment

nicoddemus Sep 17, 2024

Choose a reason for hiding this comment

nicoddemus Sep 16, 2024

Choose a reason for hiding this comment

nicoddemus Sep 16, 2024

Choose a reason for hiding this comment

nicoddemus Sep 16, 2024

Choose a reason for hiding this comment

zhukoff-pavel commented Sep 2, 2024 •

edited

Loading

nicoddemus commented Sep 9, 2024 •

edited

Loading

zhukoff-pavel commented Sep 9, 2024 •

edited

Loading