search

getsentry / sentry-python

The official Python SDK for Sentry.io
https://sentry.io/for/python/
MIT License
1.86k stars 485 forks source link

Consider mocking time in tests #3335

Open rominf opened 1 month ago

rominf commented 1 month ago

To ensure the reliability of time-dependent tests and reduce their duration, I suggest mocking time in tests.

Currently, some tests implicitly rely on underlying platform performance by setting arbitrary sleep interval duration that would be enough for tests to pass (for example, in https://github.com/getsentry/sentry-python/blame/081285897e4471690ae52b3afe81a6a495f75ec8/tests/profiler/test_continuous_profiler.py#L232). While it is possible to empirically come up with values that would make tests complete on the infrastructures used by Python SDK for Sentry.io developers, there are other infrastructures that run these tests, and their performance will be different. On faster platforms, tests will execute slower than they would if sleep delays were optimal for this specific platform. Considering thousands of tests that run a few minutes per testing workflow (most recent runs on Test Common on GitHub Actions take 6-7 minutes to complete: https://github.com/getsentry/sentry-python/actions/workflows/test-integrations-common.yml, see screenshot), speeding up tests might be valuable. On slower platforms, tests might fail and this is the original reason for filling this issue. Building Sentry SDK on Fedora infrastructure occasionally fails (https://koji.fedoraproject.org/koji/taskinfo?taskID=120920902, see screenshot) because time-dependent tests do not pass:

=================================== FAILURES ===================================
______ test_continuous_profiler_manual_start_and_stop[experiment-thread] _______
tests/profiler/test_continuous_profiler.py:234: in test_continuous_profiler_manual_start_and_stop
    assert_single_transaction_with_profile_chunks(envelopes, thread)
tests/profiler/test_continuous_profiler.py:86: in assert_single_transaction_with_profile_chunks
    assert len(items["profile_chunk"]) > 0
E   assert 0 > 0
E    +  where 0 = len([])

Another example: https://bugzilla.redhat.com/show_bug.cgi?id=2265822.

It is worth noting that this issue is not unique to Fedora infrastructure, and apparently occurs elsewhere. For example, while searching for test_continuous_profiler_manual_start_and_stop on GitHub, I noticed this test (along with other time-dependent tests) is being disabled in Gentoo package specification: https://github.com/gentoo/gentoo/blob/997938b5b7bf5deb7943b44a2dc52485d14f8076/dev-python/sentry-sdk/sentry-sdk-2.7.1.ebuild#L117-L122.

I propose mocking time. While it is possible to do using the unittest.mock library directly, I suggest using a library for that, such as https://pypi.org/project/freezegun/ or https://pypi.org/project/time-machine/. The latter currently works with CPython only, however, Sentry SDK is currently tested with CPython only as well (at least on GitHub Actions), PyPy was removed from the testing suite here: https://github.com/getsentry/sentry-python/pull/1602:

Removed PyPy from the test suite because it was never run. We have to reevaluate support for PyPy.

What is your opinion on this issue? Will you accept a PR(s) that introduces mocking for time? If so, which library do you prefer me to use?

Image Image

sl0thentr0py commented 1 month ago

hey @rominf all great points overall, yes we could definitely write a lot of our tests better that have grown over time. The gentoo link is very interesting since it points to a whole bunch of flaky and weird tests. Feel free to pick a simple library that works for you and make a PR. We will probably not prioritize this internally but if you make a contribution, we'd be happy to merge it in.