[Django] #33220: Test suite failures when using PYTHONOPTIMIZE or optimization CLI flags
Django
------------------------------------------------+------------------------
Reporter: Keryn Knight | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
I can't ''see'' any tickets for this, but searching for `assert`,
`assertion`, `-OO`, `-O2` doesn't necessarily cover all the ways to talk
about it, so apologies if this is covered elsewhere!
For reasons entirely unrelated, a while back I added the following to my
env:
{{{
export PYTHONDONTWRITEBYTECODE=true
export PYTHONUNBUFFERED=true
export PYTHONOPTIMIZE=2
export PYTHONWARNINGS=default
export PYTHONUTF8=1
}}}
Most of which have no effect, but to my consternation (because I'd
entirely forgotten having done so!), using `./runtests.py` suddenly
started throwing a lot of errors at me (because of `PYTHONOPTIMIZE=2`),
the list of which follows further below.
Most of the errors fall into either one of:
- using `assert x ==y, 'error'` being blown away entirely (in one case,
this cryptically results an `UnboundLocalError` instead)
- checking `__doc__` values, which are replaced with `None`
There are a handful of tests which fail at `-O`, and a bunch more which do
so only when `-OO` is used. A sticky issue is that there's no great answer
for CI:
- running the base suite (`./runtests.py` with none of the optional deps,
but with `-OO`) means some tests/bits won't be covered.
- running each of the supported suite combos under normal + `-OO` is
exhaustive for little benefit and many more suite executions.
- not running `-OO` at all means at best, any PR will pass without
incident (having not broken anything), but without demonstrating things
work.
The fix for `assert` would probably be to either `skipIf` them or fix the
places they're used to raise an exception which is always present, like
`ValueError` or `TypeError`, though this means changing the exceptions
outside of `tests` in a couple of places :/
The fix for `__doc__` is to either `skipIf` them or selectively test them
based on `sys.flags.optimize`.
I'd personally prefer not to `skipIf`, because many of the tests assert
multiple things, only one of which breaks.
At least one test is also, as far as I can tell, passing incorrectly
currently:
`FAIL: test_get_version_invalid_version (version.tests.VersionTests)
(version=(3, 2, 0, 'gamma', 1, '20210315111111'))`
This is actually failing on the **length** assertion rather than the
**alpha/beta/...** one.
The complete list, then, first at `-O`:
{{{
======================================================================
ERROR: test_file_error_blocking (file_uploads.tests.FileUploadTests)
The server should not block when there are upload errors (bug #8622).
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/file_uploads/tests.py", line 584, in
test_file_error_blocking
self.client.post('/upload_errors/', post_data)
File "/django/django/test/client.py", line 756, in post
response = super().post(path, data=data, content_type=content_type,
secure=secure, **extra)
File "/django/django/test/client.py", line 407, in post
return self.generic('POST', path, post_data, content_type,
File "/django/django/test/client.py", line 473, in generic
return self.request(**r)
File "/django/django/test/client.py", line 721, in request
self.check_exception(response)
File "/django/django/test/client.py", line 582, in check_exception
raise exc_value
File "/django/django/core/handlers/exception.py", line 47, in inner
response = get_response(request)
File "/django/django/core/handlers/base.py", line 181, in _get_response
response = wrapped_callback(request, *callback_args,
**callback_kwargs)
File "/django/tests/file_uploads/views.py", line 139, in
file_upload_errors
return file_upload_echo(request)
File "/django/tests/file_uploads/views.py", line 76, in file_upload_echo
r = {k: f.name for k, f in request.FILES.items()}
File "/django/django/core/handlers/wsgi.py", line 116, in FILES
self._load_post_and_files()
File "/django/django/http/request.py", line 328, in _load_post_and_files
self._post, self._files = self.parse_file_upload(self.META, data)
File "/django/django/http/request.py", line 288, in parse_file_upload
return parser.parse()
File "/django/django/http/multipartparser.py", line 262, in parse
chunk = handler.receive_data_chunk(chunk, counters[i])
File "/django/tests/file_uploads/uploadhandler.py", line 47, in
receive_data_chunk
raise CustomUploadError("Oops!")
file_uploads.uploadhandler.CustomUploadError: Oops!
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/django/tests/file_uploads/tests.py", line 585, in
test_file_error_blocking
except reference_error.__class__ as err:
UnboundLocalError: local variable 'reference_error' referenced before
assignment
======================================================================
FAIL: test_real_apps_non_set (migrations.test_state.StateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/migrations/test_state.py", line 929, in
test_real_apps_non_set
ProjectState(real_apps=['contenttypes'])
AssertionError: AssertionError not raised
======================================================================
FAIL: test_flags_with_pre_compiled_regex
(utils_tests.test_regex_helper.LazyReCompileTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/utils_tests/test_regex_helper.py", line 54, in
test_flags_with_pre_compiled_regex
lazy_test_pattern.match('TEST')
File "/python/3.9.5/lib/python3.9/contextlib.py", line 124, in __exit__
next(self.gen)
File "/django/django/test/testcases.py", line 684, in
_assert_raises_or_warns_cm
yield cm
AssertionError: AssertionError not raised
======================================================================
FAIL: test_get_version_invalid_version (version.tests.VersionTests)
(version=(3, 2, 0, 'alpha', 1, '20210315111111'))
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/version/tests.py", line 61, in
test_get_version_invalid_version
get_complete_version(version)
AssertionError: AssertionError not raised
======================================================================
FAIL: test_get_version_invalid_version (version.tests.VersionTests)
(version=(3, 2, 0, 'gamma', 1, '20210315111111'))
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/version/tests.py", line 61, in
test_get_version_invalid_version
get_complete_version(version)
AssertionError: AssertionError not raised
----------------------------------------------------------------------
Ran 15290 tests in 508.250s
FAILED (failures=4, errors=1,...
}}}
At `-OO` (ignoring those that occurred at `-O`):
{{{
======================================================================
FAIL: test_testcase_ordering
(test_runner.test_discover_runner.DiscoverRunnerTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/test_runner/test_discover_runner.py", line 301, in
test_testcase_ordering
self.assertIn('DocTestCase', [t.__class__.__name__ for t in
suite._tests[2:]])
AssertionError: 'DocTestCase' not found in ['Test', 'TestVanillaUnittest',
'SkipDocTestCase']
======================================================================
FAIL: test_migrate_check_plan (migrations.test_commands.MigrateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/django/test/utils.py", line 437, in inner
return func(*args, **kwargs)
File "/django/tests/migrations/test_commands.py", line 293, in
test_migrate_check_plan
self.assertEqual(
AssertionError: 'Plan[60 chars]alamander\n Raw Python operation -> Grow
salamander tail.\n' != 'Plan[60 chars]alamander\n Raw Python
operation\n'
Planned operations:
migrations.0001_initial
Create model Salamander
- Raw Python operation -> Grow salamander tail.
+ Raw Python operation
======================================================================
FAIL: test_migrate_plan (migrations.test_commands.MigrateTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/django/test/utils.py", line 437, in inner
return func(*args, **kwargs)
File "/django/tests/migrations/test_commands.py", line 430, in
test_migrate_plan
self.assertEqual(
AssertionError: "Plan[90 chars]ation -> Grow salamander
tail.\nmigrations.000[199 chars]']\n" != "Plan[90
chars]ation\nmigrations.0002_second\n Create mode[174 chars]']\n"
Planned operations:
migrations.0001_initial
Create model Salamander
- Raw Python operation -> Grow salamander tail.
+ Raw Python operation
migrations.0002_second
Create model Book
Raw SQL operation -> ['SELECT * FROM migrations_book']
migrations.0003_third
Create model Author
Raw SQL operation -> ['SELECT * FROM migrations_author']
======================================================================
FAIL: test_inclusion_tag_registration
(template_tests.test_custom.InclusionTagTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/template_tests/test_custom.py", line 316, in
test_inclusion_tag_registration
self.verify_tag(inclusion.inclusion_no_params, 'inclusion_no_params')
File "/django/tests/template_tests/test_custom.py", line 43, in
verify_tag
self.assertEqual(tag.__doc__, 'Expected %s __doc__' % name)
AssertionError: None != 'Expected inclusion_no_params __doc__'
======================================================================
FAIL: test_simple_tag_registration
(template_tests.test_custom.SimpleTagTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/template_tests/test_custom.py", line 155, in
test_simple_tag_registration
self.verify_tag(custom.no_params, 'no_params')
File "/django/tests/template_tests/test_custom.py", line 43, in
verify_tag
self.assertEqual(tag.__doc__, 'Expected %s __doc__' % name)
AssertionError: None != 'Expected no_params __doc__'
======================================================================
FAIL: test_preserve_attributes (decorators.tests.MethodDecoratorTests)
(Test=<class
'decorators.tests.MethodDecoratorTests.test_preserve_attributes.<locals>.TestPlain'>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/decorators/tests.py", line 271, in
test_preserve_attributes
self.assertEqual(Test.method.__doc__, 'A method')
AssertionError: None != 'A method'
======================================================================
FAIL: test_preserve_attributes (decorators.tests.MethodDecoratorTests)
(Test=<class
'decorators.tests.MethodDecoratorTests.test_preserve_attributes.<locals>.TestMethodAndClass'>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/decorators/tests.py", line 271, in
test_preserve_attributes
self.assertEqual(Test.method.__doc__, 'A method')
AssertionError: None != 'A method'
======================================================================
FAIL: test_preserve_attributes (decorators.tests.MethodDecoratorTests)
(Test=<class
'decorators.tests.MethodDecoratorTests.test_preserve_attributes.<locals>.TestFunctionIterable'>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/decorators/tests.py", line 271, in
test_preserve_attributes
self.assertEqual(Test.method.__doc__, 'A method')
AssertionError: None != 'A method'
======================================================================
FAIL: test_preserve_attributes (decorators.tests.MethodDecoratorTests)
(Test=<class
'decorators.tests.MethodDecoratorTests.test_preserve_attributes.<locals>.TestMethodIterable'>)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/decorators/tests.py", line 271, in
test_preserve_attributes
self.assertEqual(Test.method.__doc__, 'A method')
AssertionError: None != 'A method'
======================================================================
FAIL: test_cached_property (utils_tests.test_functional.FunctionalTests)
(attr='value')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/utils_tests/test_functional.py", line 63, in
assertCachedPropertyWorks
self.assertEqual(get(Class).__doc__, 'Here is the docstring...')
AssertionError: None != 'Here is the docstring...'
======================================================================
FAIL: test_cached_property (utils_tests.test_functional.FunctionalTests)
(attr='other')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/utils_tests/test_functional.py", line 63, in
assertCachedPropertyWorks
self.assertEqual(get(Class).__doc__, 'Here is the docstring...')
AssertionError: None != 'Here is the docstring...'
======================================================================
FAIL: test_cached_property (utils_tests.test_functional.FunctionalTests)
(attr='__foo__')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/utils_tests/test_functional.py", line 63, in
assertCachedPropertyWorks
self.assertEqual(get(Class).__doc__, 'Here is the docstring...')
AssertionError: None != 'Here is the docstring...'
======================================================================
FAIL: test_cached_property_auto_name
(utils_tests.test_functional.FunctionalTests) (attr='_Class__value')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/utils_tests/test_functional.py", line 63, in
assertCachedPropertyWorks
self.assertEqual(get(Class).__doc__, 'Here is the docstring...')
AssertionError: None != 'Here is the docstring...'
======================================================================
FAIL: test_cached_property_auto_name
(utils_tests.test_functional.FunctionalTests) (attr='other')
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/utils_tests/test_functional.py", line 63, in
assertCachedPropertyWorks
self.assertEqual(get(Class).__doc__, 'Here is the docstring...')
AssertionError: None != 'Here is the docstring...'
======================================================================
FAIL: test_attributes (decorators.tests.DecoratorsTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/decorators/tests.py", line 91, in test_attributes
self.assertEqual(fully_decorated.__doc__, 'Expected __doc__')
AssertionError: None != 'Expected __doc__'
----------------------------------------------------------------------
Ran 15290 tests in 480.762s
FAILED (failures=15,...
}}}
If we want to go ahead and attempt to fix/resolve these, I do have a draft
branch fixing the above errors, though it obviously won't do anything on
CI, may not catch everything, and may not be the desired way to do it
(having excised all the `assert` statements entirely, for example).
--
Ticket URL: <https://code.djangoproject.com/ticket/33220>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Keryn Knight | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => wontfix
Comment:
Raising specific errors instead of using `assert` was already discussed in
#32508. We decided that remaining cases are valid and should remain as
`assert`. I don't think it's worth changing other tests that fail with
`-O`.
--
Ticket URL: <https://code.djangoproject.com/ticket/33220#comment:1>
Django
Reporter: Keryn Knight | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob Walls):
Keryn, your point about the second subtest of
`test_get_version_invalid_version()` passing without testing what it
claims to test looks valid to me. I would hazard that would be a welcome
cleanup PR (and could ref this ticket number, even, maybe, to suggest how
it was found).
--
Ticket URL: <https://code.djangoproject.com/ticket/33220#comment:2>
Django
Reporter: Keryn Knight | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
Replying to [comment:2 Jacob Walls]:
> Keryn, your point about the second subtest of
`test_get_version_invalid_version()` passing without testing what it
claims to test looks valid to me. I would hazard that would be a welcome
cleanup PR (and could ref this ticket number, even, maybe, to suggest how
it was found).
`get_version()` is an internal Django utility, so I'd not change this (see
[https://github.com/django/django/pull/14114#discussion_r592669163
comment]).
--
Ticket URL: <https://code.djangoproject.com/ticket/33220#comment:3>
Django
Reporter: Keryn Knight | Owner: nobody
Cleanup/optimization |
Component: Testing framework | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam Johnson):
I would drop PYTHONOPTIMIZE from your environment, basically no one knows
about its existence so most code assumes `assert` always works.
--
Ticket URL: <https://code.djangoproject.com/ticket/33220#comment:4>