Opened 4 years ago

Closed 18 months ago

#18586 closed Cleanup/optimization (fixed)

Rewrite unit tests migrated from doctests

Reported by: Michal Petrucha Owned by: Michael Blatherwick
Component: Testing framework Version: master
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 Baptiste Mispelon)

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 Changed 4 years ago by anonymous

Needs documentation: unset
Needs tests: unset
Owner: changed from nobody to anonymous
Patch needs improvement: unset
Status: newassigned

comment:2 Changed 4 years ago by Mark Smith

Owner: changed from anonymous to Mark Smith
Status: assignednew

comment:3 Changed 4 years ago by Mark Smith

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

comment:4 Changed 4 years ago by Mark Smith

Owner: changed from anonymous to Mark Smith
Status: assignednew

Last attempt to get the status of this ticket correct.

comment:5 Changed 4 years ago by Mark Smith

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 Changed 4 years ago by Michal Petrucha

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 Changed 4 years ago by Aymeric Augustin

Component: UncategorizedTesting framework

comment:8 Changed 4 years ago by modi.konark@…

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 Changed 4 years ago by Claude Paroz

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 Changed 4 years ago by Carl Meyer <carl@…>

In 483e1b807e7a3c9e49fe5fcbf9b1300ef98a381c:

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

comment:11 Changed 4 years ago by Carl Meyer

Description: modified (diff)

comment:12 Changed 4 years ago by Carl Meyer

Description: modified (diff)

comment:13 Changed 4 years ago by Carl Meyer

@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 Changed 4 years ago by Claude Paroz

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 Changed 3 years ago by Tim Graham <timograham@…>

In 0d98422b1365ae1e75051036936aab31248d5ee1:

Refactored RecursiveM2MTests into smaller pieces, refs #18586.

comment:16 Changed 2 years ago by Julien Phalip

Description: modified (diff)

comment:17 Changed 2 years ago by Liav3000

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 2 years ago by Liav3000 (previous) (diff)

comment:18 Changed 2 years ago by bsilverberg

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

comment:19 Changed 2 years ago by Julien Phalip

Description: modified (diff)

comment:20 Changed 2 years ago by Peter Zsoldos

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

Version 2, edited 2 years ago by Peter Zsoldos (previous) (next) (diff)

comment:21 Changed 2 years ago by Peter Zsoldos

Has patch: set

comment:22 Changed 2 years ago by Peter Zsoldos

Cc: Peter Zsoldos added

comment:23 Changed 2 years ago by José Luis Patiño Andrés

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 Changed 2 years ago by Tim Graham <timograham@…>

In 7b064e539009b4845ca31bd4a3dd5a9b913d1a0e:

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

comment:25 Changed 2 years ago by Tim Graham <timograham@…>

In 7e2c804c9417617934cd731a4537719a1bd8d985:

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

comment:26 Changed 2 years ago by Tim Graham

Has patch: unset

comment:27 Changed 2 years ago by Tim Graham

Owner: Mark Smith deleted
Status: assignednew

comment:28 Changed 2 years ago by Anand Reddy Pandikunta

Owner: set to Anand Reddy Pandikunta
Status: newassigned

comment:29 Changed 2 years ago by Davide Ceretti

I am going to start working on M2mThroughTests

comment:30 Changed 2 years ago by Davide Ceretti

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

comment:31 Changed 2 years ago by Davide Ceretti

Description: modified (diff)

comment:32 Changed 2 years ago by Loic Bistuer <loic.bistuer@…>

In d8e157d5ab1648a509b25d5ec571572ae936de79:

Refactored m2m_through tests. Refs #18586

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

comment:33 Changed 2 years ago by Davide Ceretti

Description: modified (diff)

comment:34 Changed 2 years ago by Bruno Alla

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

comment:35 Changed 2 years ago by Tim Graham <timograham@…>

In d1e87aebf70dd035c98a0b0f0162c0a2c398598c:

Refactored model lookup tests; refs #18586.

comment:36 Changed 2 years ago by Bruno Alla

Description: modified (diff)

comment:37 Changed 23 months ago by agiliq

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

comment:38 Changed 23 months ago by agiliq

Description: modified (diff)

comment:39 Changed 23 months ago by agiliq

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

comment:40 Changed 23 months ago by agiliq

Description: modified (diff)

comment:41 Changed 23 months ago by Tim Graham

Description: modified (diff)

comment:42 Changed 22 months ago by Helen Sherwood-Taylor

PR for order_with_respect_to:
https://github.com/django/django/pull/3638

comment:43 Changed 22 months ago by Alexander Shchapov

PR for model_package.ModelPackageTests.test_models_packages: https://github.com/django/django/pull/3657

comment:44 Changed 22 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In e1513a7960399054d6686efb287f3887ad84b73f:

Fixed #18586 -- Split up model_package.ModelPackageTests.

comment:45 Changed 22 months ago by Tim Graham

Resolution: fixed
Status: closednew

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 Changed 22 months ago by Alexander Shchapov

PR for model_inheritance.ModelInheritanceTests https://github.com/django/django/pull/3658.

comment:47 Changed 22 months ago by Tim Graham <timograham@…>

In 2cd19f3738b9c4a6861c007bf22f72a386cba07f:

Refs #18586 -- Split up model_inheritance.ModelInheritanceTest

comment:48 Changed 22 months ago by Tim Graham

Description: modified (diff)

comment:49 Changed 22 months ago by Berker Peksag

PR #3638 LGTM. (I added a couple of minor comments on GitHub)

comment:50 Changed 22 months ago by Tim Graham <timograham@…>

In 7daf00679d2848fdb6f5618009817b56486fce1b:

Refs #18586 -- Split up order_with_respect_to tests

comment:51 Changed 22 months ago by Tim Graham <timograham@…>

In f66d9a2300f8fcb82d2bb21c43dc0e658c49086a:

Refs #18586 -- Split custom_pk test.

comment:52 Changed 22 months ago by Tim Graham <timograham@…>

In 6db122c7aba5738aad92aa20c9bbf2dea6d3308e:

Refs #18586 -- Split custom manager tests.

comment:53 Changed 22 months ago by Tim Graham <timograham@…>

In 43041ee48c802418ec935a2ece59f876f5888c40:

Refs #18586 -- Refactored expressions tests.

comment:54 Changed 22 months ago by Will Earp

I am going to start working on DeferTests.test_defer.

comment:55 Changed 21 months ago by Amine Zyad

Cc: amizya@… added

I'll work on m2m_signals and m2m_through_regress

comment:56 Changed 21 months ago by Tim Graham <timograham@…>

In 89527576980ff14309990c152e5904ddd5ca3df7:

Refs #18586 -- Split up tests.defer.DeferTests.test_defer

comment:57 Changed 21 months ago by Will Earp

Description: modified (diff)

comment:58 Changed 19 months ago by alainivars

Owner: changed from Anand Reddy Pandikunta to alainivars
Status: newassigned

comment:59 Changed 19 months ago by Tim Graham

Owner: alainivars deleted
Status: assignednew

comment:60 Changed 18 months ago by alainivars

Owner: set to alainivars
Status: newassigned

comment:61 Changed 18 months ago by Baptiste Mispelon <bmispelon@…>

In 82ec05f:

Refs #18586 -- Split up tests/m2m_signals

comment:62 Changed 18 months ago by Baptiste Mispelon

Description: modified (diff)

comment:63 Changed 18 months ago by Baptiste Mispelon

Owner: alainivars deleted
Status: assignednew

comment:64 Changed 18 months ago by Michael Blatherwick

Owner: set to Michael Blatherwick
Status: newassigned

comment:65 Changed 18 months ago by Michael Blatherwick

Has patch: set

comment:66 Changed 18 months ago by Tim Graham <timograham@…>

In 8654c6a7:

Refs #18586 -- Split up tests.m2m_through_regress

comment:67 Changed 18 months ago by Tim Graham

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top