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

Circumvent freezegun in favor of a real clock where possible #228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jombooth
Copy link

@jombooth jombooth commented Sep 2, 2020

Hi,

First off, thank you for this wonderful tool! At my work we've been using it to get CircleCI's Test Summary feature to display information about our test runs, and it's been awesome.

I noticed however that for certain test suites where we use Freezegun, the test runner reports a test runtime of 0.000s. I saw in this comment that some prior efforts have been made to circumvent freezegun in this project; unfortunately, this approach did not seem to work (at least, not in our case).

Following the pseudocode in this pep, I implemented a function that's typically equivalent to authentic time.time(), but unlike that function, is not mocked out by freezegun. This is now in xmlrunner.time, and used everywhere time() was previously called in this project. Where the clock_gettime call it depends on isn't available (mostly, non-Unix systems), or where there is no CLOCK_REALTIME available on the system, the new function will fall back to (possibly mocked) time.time().

I also verified in unit tests of the builder and the runner that the resulting timestamps + elapsed time evaluations are accurate even for a test during which time is frozen with freezegun - this necessitated adding freezegun to the test env in tox.ini.

tox -e pytest works fine with the new tests; I apparently didn't have some of the interpreters running the tests with tox requires, so that command failed for me locally, but I expect there won't be issues on a properly prepared system.

@coveralls
Copy link

coveralls commented Sep 2, 2020

Coverage Status

Coverage decreased (-0.2%) to 99.431% when pulling abf0ec9 on jombooth:jo-circumvent-freezegun into 7d0a0e7 on xmlrunner:master.

@codecov-commenter
Copy link

codecov-commenter commented Sep 2, 2020

Codecov Report

Merging #228 into master will decrease coverage by 0.17%.
The diff coverage is 94.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   99.61%   99.43%   -0.18%     
==========================================
  Files          15       16       +1     
  Lines        1566     1607      +41     
==========================================
+ Hits         1560     1598      +38     
- Misses          6        9       +3     
Impacted Files Coverage Δ
xmlrunner/time.py 62.50% <62.50%> (ø)
tests/builder_test.py 100.00% <100.00%> (ø)
tests/testsuite.py 100.00% <100.00%> (ø)
xmlrunner/builder.py 100.00% <100.00%> (ø)
xmlrunner/result.py 98.61% <100.00%> (ø)
xmlrunner/runner.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d0a0e7...abf0ec9. Read the comment docs.

@jombooth
Copy link
Author

Hi, just bumping this! Re: the coverage checks - I'm happy to follow up with more testing but just wanted to confirm that this change is going in a good direction so far.

@dnozay
Copy link
Member

dnozay commented Nov 16, 2020

@jombooth - can you please complete the 5 whys exercise for this PR?
e.g. why do you need to use a a real clock?

@dnozay dnozay added the needs_5_whys request is unclear and needs more information as to why we need the change label Nov 16, 2020
@jombooth
Copy link
Author

jombooth commented Nov 16, 2020

Thanks for your reply! This is maybe the second time I've ever done the "five whys" exercise, so I'll give it a shot but no guarantees this'll be the best representation of the technique:

  • Why [do I want to use a real clock]? Because currently, when I run the test suite in the large project where I'm using xmlrunner, all the tests where we use Freezegun report completing in 0.000s, both in the XML reports and on the command line. I want to get real numbers here.
  • Why? Because I'd like to understand how long my tests are actually taking, and am pretty bought into freezegun (a very popular time-mocking package - also like I mentioned, there was a previous attempt at this (bypass freezegun if installed #185)).
  • Why? Because data about slow tests can make it easier to pinpoint suites that could be optimized or removed. Data on especially fast tests can also reveal cases where the test isn't efficacious - ie, where the function called is unintentionally returning immediately or something
  • Why? Because unefficacious tests provide anti-value, falsely reassuring programmers that the code is being tested, and because slow tests make for slow CI
  • Why? Because I want my projects' tests to inspire only warranted confidence that the code is correct, and because quick CI makes for quick iteration, which makes for quick development

Copy link
Member

@dnozay dnozay left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, but would need to make freezegun dependency optional.
If I merge #231, you may need more work on this PR.

@@ -21,6 +21,7 @@ deps =
djangocurr: django>=2.2.0
pytest: pytest
lxml>=3.6.0
freezegun
Copy link
Member

Choose a reason for hiding this comment

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

this should be the same as django tests, i.e. if freezegun is not available, don't run those tests. - see other comment.


import time

def get_real_time_if_possible():
Copy link
Member

Choose a reason for hiding this comment

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

I think this snippet is better

try:  # pragma: no cover
    import freezegun

    def real_time():
        return freezegun.api.real_time()
except ImportError:  # pragma: no cover
    def real_time():
        return time.time()

https://github.com/realpython/django-slow-tests/blob/2c5673ca5b6eb093a908e4e28143d889a1893113/django_slowtests/testrunner.py#L13-L20

Copy link
Member

Choose a reason for hiding this comment

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

nit: naming with something shorter like real_time

@@ -4,8 +4,10 @@

import xml.etree.ElementTree as ET
from xml.dom.minidom import Document
from freezegun import freeze_time
Copy link
Member

Choose a reason for hiding this comment

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

try:
import django
except ImportError:
django = None
else:
from django.test.utils import get_runner
from django.conf import settings, UserSettingsHolder
from django.apps import apps
settings.configure(DEBUG=True)
TESTS_DIR = path.dirname(__file__)
@unittest.skipIf(django is None, 'django not found')
class DjangoTest(unittest.TestCase):

please use same kind of approach to not run things that depend on freezegun if it is not available

@dnozay
Copy link
Member

dnozay commented Nov 23, 2020

@jombooth , maybe this could also be the quick fix for you:

import freezegun
freezegun.configure(extend_ignore_list=['xmlrunner'])

https://github.com/spulec/freezegun#ignore-packages

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs_5_whys request is unclear and needs more information as to why we need the change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants