[Django] #34180: Document that setting language in tests affects other tests
Django
-------------------------------------+-------------------------------------
Reporter: Václav | Owner: nobody
Řehák |
Type: Bug | Status: new
Component: | Version: 4.1
Documentation | Keywords: documentation i18n
Severity: Normal | tests
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The testing documentation
https://docs.djangoproject.com/en/4.1/topics/testing/tools/#setting-the-
language suggests to set language in tests using a cookie or http header.
But it is not obvious to the reader that doing so will switch the Django
active language for all other subsequent tests.
In our case we had random test failures in parallel run depending on the
actual order of tests executed. This is easily reproducible with the
following test file
{{{
from django.contrib.auth.forms import AuthenticationForm
from django.test import TestCase
class ViewTestCase(TestCase):
def test_czech_request(self):
self.client.get("/", HTTP_ACCEPT_LANGUAGE="cs-cz")
class FormTestCase(TestCase):
def test_form(self):
f = AuthenticationForm(data={})
self.assertEqual(f.errors['username'], ['This field is
required.'])
}}}
This testsuite passes in normal run (form is tested before the view) but
fails when running tests with --reverse as the view test switches Django
to Czech language and the validation errors of the form are translated.
I'm not sure if this can be fixed (should the test runner activate the
default language before each test?) but I suggest to at least document it
and probably recommend the user to activate the default language in
tearDown method.
{{{
def tearDown(self):
translation.activate(settings.LANGUAGE_CODE)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34180>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Yes, this reproduces. Thanks!
Expected behaviour was doc'd in 3c447b108ac70757001171f7a4791f493880bf5b.
I wonder if this is a regression somewhere? It would be good to resolve as
not merely a docs fix, since otherwise the `override` method (not using
the LocaleMiddleware would be the only really feasible way.)
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
#34181 was a duplicate which found source of this behaviour:
> Django's LocaleMiddleware only activates a translation, but doesn't
deactivate it since #5241 /
>
https://github.com/django/django/commit/aa089b106b6cfc9a47cd54a0f9eb44bd44811ed9.
Given that this was for Django 1.6, ≈9 years ago now, I think we can
consider it established behaviour.
(Again, I think a note in the docs is likely option 1.)
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Raphaël Barrois):
I'll add the results of the extra investigation I did in #34181 (sorry for
not seeing this ticket, it wasn't here when I started writing mine 😅):
- The issue happened with the fix for #5241 in
https://github.com/django/django/commit/aa089b106b6cfc9a47cd54a0f9eb44bd44811ed9
- Django's `LocaleMiddleware` no longer calls `translation.deactivate()`
as part of its request cleanup.
However, the pattern shown here is very close to typical code used in
tests, where strings marked for translation are tested against the value
in the default locale.
I feel that a documentation fix stating "Every test asserting on a string
marked for translation should wrap said assertion with
`translation.override`" wouldn't actually help users, as that would add a
significant amount of boilerplate to test cases.
If the recommended solution is to call `translation.deactivate()` in each
test case' `tearDown`, I'd suggest including that in Django's source code.
Another option could be for the test client to deactivate translations
post-request when it detects that the `LocaleMiddleware` is enabled.
Otherwise, we could also explore the possibility to restore the locale
context in some post-request handler, server-side.
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: nobody
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Václav Řehák):
To fix this at the moment, we will be adding `translation.deactivate()` to
the base class of our tests. I wonder if there is any downside of
including this in Django's TestCase `tearDown`. I'm not fan of keeping the
current behavior as it can lead to difficult-to-debug random failures of
tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:4>
Django
-------------------------------------+-------------------------------------
| Naimi
Type: Bug | Status: assigned
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Faris Naimi
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Type: Bug | Status: assigned
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Faris Naimi):
The documentation was modified to explain the issue and the possibility of
activating the default language in the tearDown method.
This is now available from the branch "ticket_34180" on the repo:
https://github.com/fnaimi66/django.git
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
tests |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
How can you close ticket just because there is a commit in a Github fork?
The issue is not closed until is it merged into Django source code (or
wontfixed).
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/16397 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
Keywords: documentation i18n | Triage Stage: Accepted
tests |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
PR looks OK. Just needs tweaks.
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Type: Bug | Status: new
Component: Documentation | Version: 4.1
Severity: Normal | Resolution:
tests | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/16510 New PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: documentation i18n | Triage Stage: Ready for
tests | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"40217d1a82b0c16cddba377325d12b2c253f402a" 40217d1]:
{{{
#!CommitTicketReference repository=""
revision="40217d1a82b0c16cddba377325d12b2c253f402a"
Fixed #34180 -- Added note about resetting language in test tear-downs.
Co-authored-by: Faris Naimi <farisf...@gmail.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Type: Bug | Status: closed
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: documentation i18n | Triage Stage: Ready for
tests | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"af396ce3f9a15db27173469b1eb2388810dd00c3" af396ce]:
{{{
#!CommitTicketReference repository=""
revision="af396ce3f9a15db27173469b1eb2388810dd00c3"
[4.2.x] Fixed #34180 -- Added note about resetting language in test tear-
downs.
Co-authored-by: Faris Naimi <farisf...@gmail.com>
Backport of 40217d1a82b0c16cddba377325d12b2c253f402a from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Václav Řehák | Owner: Faris
| Naimi
Type: Bug | Status: closed
Component: Documentation | Version: 4.1
Severity: Normal | Resolution: fixed
Keywords: documentation i18n | Triage Stage: Ready for
tests | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"f586c12f0167f51c9e398d7214ac1aa5522f1460" f586c12f]:
{{{
#!CommitTicketReference repository=""
revision="f586c12f0167f51c9e398d7214ac1aa5522f1460"
[4.1.x] Fixed #34180 -- Added note about resetting language in test tear-
downs.
Co-authored-by: Faris Naimi <farisf...@gmail.com>
Backport of 40217d1a82b0c16cddba377325d12b2c253f402a from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34180#comment:14>