Opened 12 years ago
Closed 10 years ago
#18586 closed Cleanup/optimization (fixed)
Rewrite unit tests migrated from doctests
Reported by: | Michal Petrucha | Owned by: | Michael Blatherwick |
---|---|---|---|
Component: | Testing framework | Version: | dev |
Severity: | Normal | Keywords: | unit tests |
Cc: | Peter Zsoldos, amizya@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description (last modified by )
There's quite a lot of unit tests that have been rewritten 1:1 from doctests. These tests tend to verify all aspects of a certain feature at once, they modify the state of the database throughout the whole test and by the end of the test no one really knows what the database state is supposed to be.
For example, tests.generic_relations.GenericRelationsTests.test_generic_relations
walks through most of the features supported by GenericForeignKeys
, it is 130 lines long and in case the last assertion fails, you'll have to spend ridiculous amounts of time figuring out what happened.
A likely incomplete list of similar tests:
tests.basic.ModelTest.test_lookup
tests.basic.ModelTest.test_object_creation
tests.custom_columns.CustomColumnsTests.test_db_column
tests.custom_managers.CustomManagerTests.test_manager
tests.custom_pk.CustomPKTests.test_custom_pk
tests.defer.DeferTests.test_defer
tests.expressions.ExpressionsTests.test_filter
tests.field_subclassing.CustomField.test_custom_field (deprecated)
tests.get_or_create.GetOrCreateTests.test_get_or_create
tests.m2m_recursive.RecursiveM2MTests.test_recursive_m2m
tests.m2m_signals.ManyToManySignalsTest
tests.m2m_through.M2mThroughTests
tests.m2m_through_regress.M2MThroughTestCase
tests.model_inheritance.ModelInheritanceTests
tests.model_package.ModelPackageTests.test_model_packages
tests.order_with_respect_to.OrderWithRespectToTests.test_basic
tests.signals.SignalTests.test_basic
This ticket is meant to track all of them; each time a test is updated, we can strike it out in the list above and once there is no item left, we can close this. The point is to make the test suite help developers as much as possible.
Change History (67)
comment:1 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:3 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
comment:5 by , 12 years ago
Status: | new → assigned |
---|
Basic test refactorings now committed at https://github.com/bedmondmark/django/tree/test_refactor - I'll issue a Pull Request when I've finished the whole lot.
comment:6 by , 12 years ago
Found another candidate: regressiontests.defer_regress.DeferRegressionTest.test_basic
IMO, this test is broken because its results depend on the order of execution of tests as it accesses the global model class storage. This means it won't be that easy to rewrite this test.
comment:7 by , 12 years ago
Component: | Uncategorized → Testing framework |
---|
comment:8 by , 12 years ago
Tried Merging the pull request by koniiik, there were lot of conflicts. The test directory layout has changed and also the conventions used needed to be re-worked.
Will be re-writing the tests and issue a pull request as and when they are done.
https://github.com/django/django/pull/923 , for the test " Custom Columns " .
comment:9 by , 12 years ago
I'd just like to note that cutting single tests in multiple tests has a cost for the test suite, as the database is flushed between tests. So it's right and fine to separate tests when testing different features, but don't just create test methods for each single database request.
comment:11 by , 12 years ago
Description: | modified (diff) |
---|
comment:12 by , 12 years ago
Description: | modified (diff) |
---|
comment:13 by , 12 years ago
@claudep If you want to look over the initial PR I just committed and comment on whether you think these tests are split up too finely, I'd like to make sure we have consensus on the right level of test granularity before @modi.konark moves forward with more PRs.
comment:14 by , 12 years ago
Carl, I guess we're also entering personal style here.
For example in that commit, I would have regrouped test_get_first_name
/test_filter_first_name
and probably also the 3 last (test_get_all_authors_for_an_article
/test_get_all_articles_for_an_author
/test_get_author_m2m_relation
), particularly as they are only querying data.
But once again, this is just me, and I can understand others have different approaches.
comment:16 by , 11 years ago
Description: | modified (diff) |
---|
comment:17 by , 11 years ago
Hi. I just created https://github.com/liavkoren-vmfarms/djangoDev/tree/ticket_18586 which refactors modeltests.get_or_create.GetOrCreateTests.test_get_or_create into smaller units and changes comments over to docstrings.
I just created a pull request for this patch.
comment:19 by , 11 years ago
Description: | modified (diff) |
---|
comment:20 by , 10 years ago
I'm at the DjangoCon Europe Sprint and working on splitting up modeltests.basic.ModelTest.test_object_creation
(by the way, now it's called basic.tests.ModelTest.test_object_creation
)
Patch is at https://github.com/django/django/pull/2671
[update: pull requests is now squashed into a single commit]
comment:21 by , 10 years ago
Has patch: | set |
---|
comment:22 by , 10 years ago
Cc: | added |
---|
comment:23 by , 10 years ago
I have committed a pull request for the first reported unit test modeltests.generic_relations.GenericRelationsTests.test_generic_relations
. You can check the PR in Github.
comment:26 by , 10 years ago
Has patch: | unset |
---|
comment:27 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:28 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:30 by , 10 years ago
Patch for M2mThroughTests refactor is at https://github.com/django/django/pull/3273
comment:31 by , 10 years ago
Description: | modified (diff) |
---|
comment:33 by , 10 years ago
Description: | modified (diff) |
---|
comment:34 by , 10 years ago
Patch for ModelTest.test_lookup: https://github.com/django/django/pull/3306
comment:36 by , 10 years ago
Description: | modified (diff) |
---|
comment:37 by , 10 years ago
Opened PR for custom pks here:
https://github.com/django/django/pull/3483
comment:38 by , 10 years ago
Description: | modified (diff) |
---|
comment:39 by , 10 years ago
Opened PR for custom_managers here:
https://github.com/django/django/pull/3491
comment:40 by , 10 years ago
Description: | modified (diff) |
---|
comment:41 by , 10 years ago
Description: | modified (diff) |
---|
comment:42 by , 10 years ago
PR for order_with_respect_to:
https://github.com/django/django/pull/3638
comment:43 by , 10 years ago
PR for model_package.ModelPackageTests.test_models_packages: https://github.com/django/django/pull/3657
comment:44 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:45 by , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
Woops, should have been "refs #" in the commit message of that last one so this didn't get closed as there is still more work to do in the other tests.
comment:46 by , 10 years ago
PR for model_inheritance.ModelInheritanceTests https://github.com/django/django/pull/3658.
comment:48 by , 10 years ago
Description: | modified (diff) |
---|
comment:57 by , 10 years ago
Description: | modified (diff) |
---|
comment:58 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:59 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:60 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:62 by , 10 years ago
Description: | modified (diff) |
---|
comment:63 by , 10 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:64 by , 10 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:67 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Last attempt to get the status of this ticket correct.