Opened 12 years ago

Last modified 10 years ago

#18586 closed Cleanup/optimization

Rewrite unit tests migrated from doctests — at Version 40

Reported by: Michal Petrucha Owned by: ChillarAnand
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 agiliq)

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, modeltests.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:
modeltests.basic.ModelTest.test_lookup
modeltests.basic.ModelTest.test_object_creation
modeltests.custom_columns.CustomColumnsTests.test_db_column
modeltests.custom_managers.CustomManagerTests.test_manager
modeltests.custom_pk.CustomPKTests.test_custom_pk
modeltests.defer.DeferTests.test_defer
modeltests.expressions.ExpressionsTests.test_filter
modeltests.field_subclassing.CustomField.test_custom_field
modeltests.files.FileStorageTests.test_files
modeltests.get_or_create.GetOrCreateTests.test_get_or_create
modeltests.m2m_recursive.RecursiveM2MTests.test_recursive_m2m
modeltests.m2m_signals.ManyToManySignalsTest
modeltests.m2m_through.M2mThroughTests
modeltests.m2m_through_regress.M2MThroughTestCase
modeltests.model_formsets.ModelFormsetTests
modeltests.model_forms.OldFormForXTests.test_with_data (This one has more than 500 lines!)
modeltests.model_inheritance.ModelInheritanceTests
modeltests.model_package.ModelPackageTests.test_model_packages
modeltests.order_with_respect_to.OrderWithRespectToTests.test_basic
modeltests.signals.SignalTests.test_basic

I only went through modeltests/*/tests.py; I might have overlooked a few tests.

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 (40)

comment:1 by anonymous, 12 years ago

Owner: changed from nobody to anonymous
Status: newassigned

comment:2 by Mark Smith, 12 years ago

Owner: changed from anonymous to Mark Smith
Status: assignednew

comment:3 by Mark Smith, 12 years ago

Owner: changed from Mark Smith to anonymous
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:4 by Mark Smith, 12 years ago

Owner: changed from anonymous to Mark Smith
Status: assignednew

Last attempt to get the status of this ticket correct.

comment:5 by Mark Smith, 12 years ago

Status: newassigned

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 Michal Petrucha, 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 Aymeric Augustin, 12 years ago

Component: UncategorizedTesting framework

comment:8 by modi.konark@…, 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 Claude Paroz, 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:10 by Carl Meyer <carl@…>, 12 years ago

In 483e1b807e7a3c9e49fe5fcbf9b1300ef98a381c:

Refs #18586 -- Split out long custom_columns lookup test into multiple tests.

comment:11 by Carl Meyer, 12 years ago

Description: modified (diff)

comment:12 by Carl Meyer, 12 years ago

Description: modified (diff)

comment:13 by Carl Meyer, 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 Claude Paroz, 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:15 by Tim Graham <timograham@…>, 11 years ago

In 0d98422b1365ae1e75051036936aab31248d5ee1:

Refactored RecursiveM2MTests into smaller pieces, refs #18586.

comment:16 by Julien Phalip, 11 years ago

Description: modified (diff)

comment:17 by Liav3000, 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.

Last edited 11 years ago by Liav3000 (previous) (diff)

comment:18 by bsilverberg, 11 years ago

I am going to start working on modeltests.basic.ModelTest.test_lookup

comment:19 by Julien Phalip, 11 years ago

Description: modified (diff)

comment:20 by Peter Zsoldos, 11 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]

Last edited 10 years ago by Peter Zsoldos (previous) (diff)

comment:21 by Peter Zsoldos, 11 years ago

Has patch: set

comment:22 by Peter Zsoldos, 11 years ago

Cc: Peter Zsoldos added

comment:23 by José Luis Patiño Andrés, 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:24 by Tim Graham <timograham@…>, 10 years ago

In 7b064e539009b4845ca31bd4a3dd5a9b913d1a0e:

Split GenericRelationsTests.test_generic_relations into several tests; refs #18586.

comment:25 by Tim Graham <timograham@…>, 10 years ago

In 7e2c804c9417617934cd731a4537719a1bd8d985:

Split tests.basic.ModelTests in several tests; refs #18586.

comment:26 by Tim Graham, 10 years ago

Has patch: unset

comment:27 by Tim Graham, 10 years ago

Owner: Mark Smith removed
Status: assignednew

comment:28 by ChillarAnand, 10 years ago

Owner: set to ChillarAnand
Status: newassigned

comment:29 by Davide Ceretti, 10 years ago

I am going to start working on M2mThroughTests

comment:30 by Davide Ceretti, 10 years ago

Patch for M2mThroughTests refactor is at https://github.com/django/django/pull/3273

comment:31 by Davide Ceretti, 10 years ago

Description: modified (diff)

comment:32 by Loic Bistuer <loic.bistuer@…>, 10 years ago

In d8e157d5ab1648a509b25d5ec571572ae936de79:

Refactored m2m_through tests. Refs #18586

Refactored old tests that were rewritten 1:1 from doctests.

comment:33 by Davide Ceretti, 10 years ago

Description: modified (diff)

comment:34 by Bruno Alla, 10 years ago

Patch for ModelTest.test_lookup: https://github.com/django/django/pull/3306

comment:35 by Tim Graham <timograham@…>, 10 years ago

In d1e87aebf70dd035c98a0b0f0162c0a2c398598c:

Refactored model lookup tests; refs #18586.

comment:36 by Bruno Alla, 10 years ago

Description: modified (diff)

comment:37 by agiliq, 10 years ago

Opened PR for custom pks here:
https://github.com/django/django/pull/3483

comment:38 by agiliq, 10 years ago

Description: modified (diff)

comment:39 by agiliq, 10 years ago

Opened PR for custom_managers here:
https://github.com/django/django/pull/3491

comment:40 by agiliq, 10 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top