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

Fix -vv overriding --durations-min (#12938) #13100

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

Conversation

JulianJvn
Copy link

Fix --durations-min argument not respected if -vv is used.

Fixes #12938.

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jan 2, 2025
@JulianJvn JulianJvn marked this pull request as draft January 2, 2025 07:05
@JulianJvn JulianJvn force-pushed the durations-min-precedence branch from 7e06d44 to 7e5c1b6 Compare January 2, 2025 07:33

tested = "3"
for x in tested:
for y in ("call",): # 'setup', 'call', 'teardown':
Copy link
Author

Choose a reason for hiding this comment

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

Not sure what the rationale for this comment is. It was added with --durations in 3b9fd3a, 13 years ago. I removed it.


for x in "123":
for y in ("call",): # 'setup', 'call', 'teardown':
Copy link
Author

Choose a reason for hiding this comment

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

Looking for "call" is not sufficient, as the output in question contains test_calls_showall_verbose.py::test_X PASSED (e.g.) for all test numbers X, which contains call as well, even though the call might be missing in the durations list. Added a space before and after call to fix this.

Comment on lines -974 to -975
tested = "3"
for x in tested:
Copy link
Author

Choose a reason for hiding this comment

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

This only checks whether test 3 is in the output, but not if the other tests are not. Fixed this.

break
else:
raise AssertionError(f"not found {x} {y}")
def test_calls_showall_durationsmin(self, pytester: Pytester, mock_timing) -> None:
Copy link
Author

Choose a reason for hiding this comment

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

A test for --durations-min was missing. Added one.

assert result.ret == 0
TestDurations.check_tests_in_output(result.stdout.lines, "3")

def test_calls_showall_durationsmin_verbose(
Copy link
Author

Choose a reason for hiding this comment

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

This is the actual test that fails without the change in src/_pytest/runner.py.

TestDurations.check_tests_in_output(result.stdout.lines, "3")

@staticmethod
def check_tests_in_output(lines: Sequence[str], expected_test_numbers: str) -> None:
Copy link
Author

Choose a reason for hiding this comment

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

Reduced code duplication by refactoring common code into a separate function.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to take expected_test_numbers: Collection[int], or better yet *expected_test_numbers: int, then check for more than just 1/2/3 in the condition below, and finally assert a comparison between eg lists.

@JulianJvn JulianJvn marked this pull request as ready for review January 2, 2025 08:03
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Just a few things I noticed. Not a complete review.

src/_pytest/runner.py Outdated Show resolved Hide resolved
src/_pytest/runner.py Outdated Show resolved Hide resolved
@JulianJvn JulianJvn force-pushed the durations-min-precedence branch from d8ccca7 to a811765 Compare January 2, 2025 17:45
@JulianJvn JulianJvn requested review from webknjaz January 2, 2025 18:19
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

The change itself and your fixes to the tests look good to me - thanks! One comment below about clarifying the helper function, and then I think we're ready to merge 🙂

TestDurations.check_tests_in_output(result.stdout.lines, "3")

@staticmethod
def check_tests_in_output(lines: Sequence[str], expected_test_numbers: str) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to take expected_test_numbers: Collection[int], or better yet *expected_test_numbers: int, then check for more than just 1/2/3 in the condition below, and finally assert a comparison between eg lists.

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.

-vv should not override --durations-min
3 participants