Opened 3 years ago

Closed 5 months ago

#18586 closed Cleanup/optimization (fixed)

Rewrite unit tests migrated from doctests

Reported by: koniiiik Owned by: exonian
Component: Testing framework Version: master
Severity: Normal Keywords: unit tests
Cc: zsoldosp, 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 bmispelon)

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

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to anonymous
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 3 years ago by judy2k

  • Owner changed from anonymous to judy2k
  • Status changed from assigned to new

comment:3 Changed 3 years ago by judy2k

  • Owner changed from judy2k to anonymous
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 years ago by judy2k

  • Owner changed from anonymous to judy2k
  • Status changed from assigned to new

Last attempt to get the status of this ticket correct.

comment:5 Changed 3 years ago by judy2k

  • Status changed from new to 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 Changed 3 years ago by koniiiik

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 3 years ago by aaugustin

  • Component changed from Uncategorized to Testing framework

comment:8 Changed 2 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 2 years ago by claudep

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

In 483e1b807e7a3c9e49fe5fcbf9b1300ef98a381c:

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

comment:11 Changed 2 years ago by carljm

  • Description modified (diff)

comment:12 Changed 2 years ago by carljm

  • Description modified (diff)

comment:13 Changed 2 years ago by carljm

@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 2 years ago by claudep

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 19 months ago by Tim Graham <timograham@…>

In 0d98422b1365ae1e75051036936aab31248d5ee1:

Refactored RecursiveM2MTests into smaller pieces, refs #18586.

comment:16 Changed 17 months ago by julien

  • Description modified (diff)

comment:17 Changed 17 months 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'd be happy to continue crunching through the tests in this ticket if everything is okay with that first set of changes.

Thanks, Liav.

Version 0, edited 17 months ago by Liav3000 (next)

comment:18 Changed 17 months ago by bsilverberg

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

comment:19 Changed 17 months ago by julien

  • Description modified (diff)

comment:20 Changed 16 months ago by zsoldosp

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 15 months ago by zsoldosp (previous) (diff)

comment:21 Changed 16 months ago by zsoldosp

  • Has patch set

comment:22 Changed 16 months ago by zsoldosp

  • Cc zsoldosp added

comment:23 Changed 16 months ago by jose

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 15 months ago by Tim Graham <timograham@…>

In 7b064e539009b4845ca31bd4a3dd5a9b913d1a0e:

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

comment:25 Changed 14 months ago by Tim Graham <timograham@…>

In 7e2c804c9417617934cd731a4537719a1bd8d985:

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

comment:26 Changed 14 months ago by timo

  • Has patch unset

comment:27 Changed 13 months ago by timo

  • Owner judy2k deleted
  • Status changed from assigned to new

comment:28 Changed 13 months ago by ChillarAnand

  • Owner set to ChillarAnand
  • Status changed from new to assigned

comment:29 Changed 11 months ago by davide-ceretti

I am going to start working on M2mThroughTests

comment:30 Changed 11 months ago by davide-ceretti

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

comment:31 Changed 11 months ago by davide-ceretti

  • Description modified (diff)

comment:32 Changed 11 months 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 11 months ago by davide-ceretti

  • Description modified (diff)

comment:34 Changed 11 months ago by browniebroke

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

comment:35 Changed 11 months ago by Tim Graham <timograham@…>

In d1e87aebf70dd035c98a0b0f0162c0a2c398598c:

Refactored model lookup tests; refs #18586.

comment:36 Changed 11 months ago by browniebroke

  • Description modified (diff)

comment:37 Changed 10 months ago by agiliq

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

comment:38 Changed 10 months ago by agiliq

  • Description modified (diff)

comment:39 Changed 10 months ago by agiliq

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

comment:40 Changed 10 months ago by agiliq

  • Description modified (diff)

comment:41 Changed 9 months ago by timgraham

  • Description modified (diff)

comment:42 Changed 9 months ago by helenst

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

comment:43 Changed 9 months ago by alexanderad

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

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

  • Resolution set to fixed
  • Status changed from assigned to closed

In e1513a7960399054d6686efb287f3887ad84b73f:

Fixed #18586 -- Split up model_package.ModelPackageTests.

comment:45 Changed 9 months ago by timgraham

  • Resolution fixed deleted
  • Status changed from closed to 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 Changed 9 months ago by alexanderad

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

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

In 2cd19f3738b9c4a6861c007bf22f72a386cba07f:

Refs #18586 -- Split up model_inheritance.ModelInheritanceTest

comment:48 Changed 9 months ago by timgraham

  • Description modified (diff)

comment:49 Changed 9 months ago by berkerpeksag

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

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

In 7daf00679d2848fdb6f5618009817b56486fce1b:

Refs #18586 -- Split up order_with_respect_to tests

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

In f66d9a2300f8fcb82d2bb21c43dc0e658c49086a:

Refs #18586 -- Split custom_pk test.

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

In 6db122c7aba5738aad92aa20c9bbf2dea6d3308e:

Refs #18586 -- Split custom manager tests.

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

In 43041ee48c802418ec935a2ece59f876f5888c40:

Refs #18586 -- Refactored expressions tests.

comment:54 Changed 9 months ago by wearp

I am going to start working on DeferTests.test_defer.

comment:55 Changed 8 months ago by AmiZya

  • Cc amizya@… added

I'll work on m2m_signals and m2m_through_regress

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

In 89527576980ff14309990c152e5904ddd5ca3df7:

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

comment:57 Changed 8 months ago by wearp

  • Description modified (diff)

comment:58 Changed 6 months ago by alainivars

  • Owner changed from ChillarAnand to alainivars
  • Status changed from new to assigned

comment:59 Changed 6 months ago by timgraham

  • Owner alainivars deleted
  • Status changed from assigned to new

comment:60 Changed 5 months ago by alainivars

  • Owner set to alainivars
  • Status changed from new to assigned

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

In 82ec05f:

Refs #18586 -- Split up tests/m2m_signals

comment:62 Changed 5 months ago by bmispelon

  • Description modified (diff)

comment:63 Changed 5 months ago by bmispelon

  • Owner alainivars deleted
  • Status changed from assigned to new

comment:64 Changed 5 months ago by exonian

  • Owner set to exonian
  • Status changed from new to assigned

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

In 8654c6a7:

Refs #18586 -- Split up tests.m2m_through_regress

comment:67 Changed 5 months ago by timgraham

  • Resolution set to fixed
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.
Back to Top