Code

Opened 22 months ago

Last modified 7 days ago

#18586 assigned Cleanup/optimization

Rewrite unit tests migrated from doctests

Reported by: koniiiik Owned by: judy2k
Component: Testing framework Version: master
Severity: Normal Keywords: unit tests
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by julien)

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.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.

Attachments (0)

Change History (19)

comment:1 Changed 22 months 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 22 months ago by judy2k

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

comment:3 Changed 22 months 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 22 months 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 22 months 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 22 months 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 16 months ago by aaugustin

  • Component changed from Uncategorized to Testing framework

comment:8 Changed 13 months 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 13 months 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 13 months ago by Carl Meyer <carl@…>

In 483e1b807e7a3c9e49fe5fcbf9b1300ef98a381c:

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

comment:11 Changed 13 months ago by carljm

  • Description modified (diff)

comment:12 Changed 13 months ago by carljm

  • Description modified (diff)

comment:13 Changed 13 months 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 13 months 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 2 months ago by Tim Graham <timograham@…>

In 0d98422b1365ae1e75051036936aab31248d5ee1:

Refactored RecursiveM2MTests into smaller pieces, refs #18586.

comment:16 Changed 7 days ago by julien

  • Description modified (diff)

comment:17 Changed 7 days 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 7 days ago by Liav3000 (previous) (diff)

comment:18 Changed 7 days ago by bsilverberg

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

comment:19 Changed 7 days ago by julien

  • Description modified (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as assigned
The owner will be changed from judy2k to anonymous. Next status will be 'assigned'
The ticket will be disowned. Next status will be 'new'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.