[Django] #23792: Random test failure of TestCookieStorage.test_reset_cookie
Django
--------------------------------------+--------------------
Reporter: tchaumeny | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
This test fails randomly, as seen in http://djangoci.com/job/django-pull-
requests/1528/database=postgres,python=python3.4/testReport/junit/django.contrib.formtools.tests.wizard.test_cookiestorage/TestCookieStorage/test_reset_cookie/
After some investigation, I can reproduce the failure by adding a
`time.sleep(1)` after this line
https://github.com/django/django/blob/88b2a20f047347da86f448fe09a56251d29e4168/django/core/signing.py#L183.
I suggest the introduction of a context manager `freeze_time` to handle
this issue (there is also a recurring pattern for that in Django test
suite).
--
Ticket URL: <https://code.djangoproject.com/ticket/23792>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: tchaumeny | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Uncategorized | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tchaumeny):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
> This test fails randomly, as seen in http://djangoci.com/job/django-pull-
> requests/1528/database=postgres,python=python3.4/testReport/junit/django.contrib.formtools.tests.wizard.test_cookiestorage/TestCookieStorage/test_reset_cookie/
>
> After some investigation, I can reproduce the failure by adding a
> `time.sleep(1)` after this line
> https://github.com/django/django/blob/88b2a20f047347da86f448fe09a56251d29e4168/django/core/signing.py#L183.
>
> I suggest the introduction of a context manager `freeze_time` to handle
> this issue (there is also a recurring pattern for that in Django test
> suite).
New description:
This test fails randomly, as seen in http://djangoci.com/job/django-pull-
requests/1528/database=postgres,python=python3.4/testReport/junit/django.contrib.formtools.tests.wizard.test_cookiestorage/TestCookieStorage/test_reset_cookie/
After some investigation, I can reproduce the failure by adding a
`time.sleep(1)` after this line
https://github.com/django/django/blob/88b2a20f047347da86f448fe09a56251d29e4168/django/core/signing.py#L183.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: tchaumeny | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* component: Uncategorized => Testing framework
* stage: Unreviewed => Accepted
Comment:
The contextmanager approach looks good, but formtools is being split into
a separate repo (#23677). If formtools is going to use this, then I guess
we should make this a public API. What do you think?
I created [https://github.com/django/django-formtools/issues/30 a ticket
in the django-formtools] issue tracker.
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:2>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by jezdez):
Someone proposed freezegun in the PR
(https://github.com/spulec/freezegun), which seems like the more complete
approach than this local monkey patch. So to me the question is whether we
accept the limitation of the context manager since it's a test utility but
should point at freezegun as well?
For formtools I'd like to follow whatever we decide for Django and then
backport it for versions that support older Djangos.
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:3>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
Our current policy for test dependencies is to skip any tests when the
dependencies aren't installed, so I'm not sure if adding a dependency is
worth it for the affected tests. On the other hand, if freezegun could be
used more often in our tests (haven't looked), then maybe it's worth
trying to change the policy and make it a hard dependency. Making mock a
hard dependency was proposed in #23289. I think the state of virtualenv,
etc. is at a point where we could make this a requirement, but I'll write
to the mailing list about it if there are no immediate objections here.
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:4>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by jezdez):
Makes sense to me, thanks Tim!
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:5>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Marking as "patch needs improvement" to indicate the needed research as to
whether or not to use freezegun (commented on the PR about this).
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:6>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"994d6137a2519436d17d5b3d16cb49a3fa79f93e"]:
{{{
#!CommitTicketReference repository=""
revision="994d6137a2519436d17d5b3d16cb49a3fa79f93e"
Fixed #23792 -- Added test.utils.freeze_time() context manager.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:7>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: closed
Component: Testing framework | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Just to summarize the conclusions on the pull request, we decided that
adding freezegun as a dependency wasn't worth the two tests that could use
it. The `freeze_time()` context manager has some limitations but is fine
for Django's test suite and should be fine to use in formtools tests even
though we didn't make it a public API. I don't think the fact that we'd
like to use it their justifies bypassing our normal backport policy and
and adding it to 1.7 though.
--
Ticket URL: <https://code.djangoproject.com/ticket/23792#comment:8>