Skip to content

Conversation

@deyshift
Copy link
Contributor

This pull request addresses issue #13384, where time.time() was causing pytest to report negative durations due to its susceptibility to system clock adjustments.

The following changes were made:

  • Replaced all occurrences of time.time() with _pytest.timing.perf_counter() for more reliable and monotonic timing.

  • Updated the relevant tests to use _pytest.timing.perf_counter() where applicable.

  • Added a changelog entry (13384.bugfix.rst) to document the fix.

  • Added myself to the AUTHORS file in alphabetical order.

  • Closes pytest can report negative duration due to usage of time.time() #13384

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Apr 25, 2025
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.

To ensure we can use the time mocking

We should use module imports only

@deyshift
Copy link
Contributor Author

Thank you, the requested changes have been made!

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.

Thanks @rivtechprojects, but this is incomplete, only the calls in the test files have been changed: we need to update all the places in production code as well, as listed in the original issue.

@deyshift
Copy link
Contributor Author

deyshift commented Apr 25, 2025

@nicoddemus

Thank you for pointing this out! I see in runner.py we have a start = timing.time() and a precise_start = timing.perf_counter(). What is the recommendation? Keep both and change start/stop to use perf_counter or just use precise_time?

image

@deyshift deyshift requested a review from nicoddemus April 25, 2025 15:10
@deyshift
Copy link
Contributor Author

@nicoddemus Apologies for the premature review request. It was unintended. I will update the pr once the above question is addressed.

@nicoddemus
Copy link
Member

What is the recommendation? Keep both and change start/stop to use perf_counter or just use precise_time?

Those don't need changing: we need time.time() for start and stop because they represent real user time, while perf_counter() has no guarantee. In that snippet, duration is already computed correctly using perf_counter.

#6939 (comment)

@deyshift
Copy link
Contributor Author

deyshift commented Apr 25, 2025

@nicoddemus @RonnyPfannschmidt

Thank you for all the feedback, requested changes have been pushed! However, it appears some tests are now failing related to the changes in junitxml.py.

Due to these tests failing, I propose we revert suite_start_time to use to timing.time() or datetime.now(timezone.utc) and add a suite_start_pref variable for measurements.

The test for junitxml passes with these adjustments

image

…ite start time, but retain perf_counter to measure suite_time_delta for precise calculations.
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.

Thanks @rivtechprojects!

I will take the liberty of reverting one of the changes as per my suggestion.

@nicoddemus
Copy link
Member

I will leave this open for a few days in case somebody wants to chime in.

Note: we should squash this before merging.

@nicoddemus
Copy link
Member

cc @RonnyPfannschmidt

@nicoddemus nicoddemus merged commit 8173aa8 into pytest-dev:main Apr 27, 2025
28 checks passed
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.

pytest can report negative duration due to usage of time.time()

3 participants