Django

Code

Ticket #4371 (closed: fixed)

Opened 1 year ago

Last modified 3 months ago

fixture loading fails silently in testcases

Reported by: John Shaffer <jshaffer2112@gmail.com> Assigned to: keithb
Milestone: Component: Unit test system
Version: SVN Keywords: fixtures sprintsept14
Cc: kbussell@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

Recently Satchmo gained some unittests that load yaml fixtures. I didn't have PyYAML installed, so the effect was for all the tests to fail horribly. With "manage.py loaddata" the errors are shown, but I think that these errors should be reported by the testing framework.

Attachments

raise_verbosity.diff (473 bytes) - added by John Shaffer <jshaffer2112@gmail.com> on 05/23/07 13:42:54.
Simply raise the verbosity level. Probably shows more messages than anyone will want.
dont_suppress_skip_messages.diff (0.8 kB) - added by John Shaffer <jshaffer2112@gmail.com> on 05/23/07 13:47:06.
A better solution, IMO: Show "skipping fixture" messages even when verbosity is 0. You may want to change this to use whatever Django's error conventions are, rather than a plain print statement.
russels_070913_suggestions.diff (3.1 kB) - added by keithb on 09/14/07 19:32:33.
Patch for django.core.management.commands.loaddata.py
fixtures_test.diff (1.3 kB) - added by keithb on 09/14/07 19:37:50.
Patch for tests.modeltests.fixtures
Bug4371_take2.diff (3.4 kB) - added by keithb on 11/12/07 18:49:58.
Combined patch for fix and test
bad_fixture1.unkn (60 bytes) - added by keithb on 11/12/07 18:57:26.
Goes in django/tests/regressiontests/fixtures_regress/fixtures
bad_fixture2.xml (300 bytes) - added by keithb on 11/12/07 18:57:45.
Goes in django/tests/regressiontests/fixtures_regress/fixtures

Change History

05/23/07 13:42:54 changed by John Shaffer <jshaffer2112@gmail.com>

  • attachment raise_verbosity.diff added.

Simply raise the verbosity level. Probably shows more messages than anyone will want.

05/23/07 13:47:06 changed by John Shaffer <jshaffer2112@gmail.com>

  • attachment dont_suppress_skip_messages.diff added.

A better solution, IMO: Show "skipping fixture" messages even when verbosity is 0. You may want to change this to use whatever Django's error conventions are, rather than a plain print statement.

06/01/07 20:31:44 changed by russellm

  • owner changed from jacob to russellm.
  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

06/09/07 12:50:21 changed by curtis.thompson@gmail.com

+1 on the raise_verbosity patch. Without this patch, there is no way to tell if your test specific fixtures are being loaded. This is especially confusing since 'initial_data' fixture loading is displayed.

09/13/07 08:26:45 changed by russellm

  • component changed from Uncategorized to Unit test system.
  • stage changed from Unreviewed to Accepted.

I agree that this class of error should be caught. However, just upping the verbosity level isn't the fix. Tests shouldn't generate errors unless something goes wrong.

The dont_suppress.. patch is better. However:

  1. It needs to be updated for the recent management.py refactor
  2. I would argue that if you attempt to load fixture.foo, and the foo format isn't supported, it shouldn't just be a message - it should be a full error.
  3. The patch doesn't address the issue of errors caused by loading fixtures with an implied format. i.e., if you specify loaddata myfixture, and you have myfixture.foo, but the foo format isn't valid, no fixtures will be loaded, but the provided patch won't catch the error.

Point 3 is a little hard to catch completely; however, you could do a reasonable job by raising an error if a fixture is named, but no objects are loaded as a result.

09/14/07 12:09:26 changed by keithb

  • owner changed from nobody to keithb.

09/14/07 19:32:33 changed by keithb

  • attachment russels_070913_suggestions.diff added.

Patch for django.core.management.commands.loaddata.py

09/14/07 19:37:50 changed by keithb

  • attachment fixtures_test.diff added.

Patch for tests.modeltests.fixtures

09/14/07 19:44:11 changed by keithb

  • keywords changed from fixtures to fixtures sprintsept14.

Ready for review

09/27/07 09:01:37 changed by russellm

  • needs_better_patch set to 1.

This is close, but not quite ready. In particular, the 'empty' error handling isn't quite right (or at least, is only one of a possible kind of error). The original case (3) that I gave isn't covered by your tests; I suspect all that is required is to move the 'if empty' check one level less of indentation, so that you are checking if any objects have been installed for a given fixture name, rather than checking if a specific file has no objects in it.

Some other minor nitpicks:

  • could you please produce diffs from the root directory, rather than at the file level
  • The test case should really be in the regression tests, not the model tests. The test doesn't serve any real documenation purpose; it is only to validate that the error conditions are correctly caught.

10/16/07 14:25:04 changed by keithb

  • cc changed from jshaffer2112@gmail.com to jshaffer2112@gmail.com, kbussell@gmail.com.

11/12/07 18:49:58 changed by keithb

  • attachment Bug4371_take2.diff added.

Combined patch for fix and test

11/12/07 18:57:26 changed by keithb

  • attachment bad_fixture1.unkn added.

Goes in django/tests/regressiontests/fixtures_regress/fixtures

11/12/07 18:57:45 changed by keithb

  • attachment bad_fixture2.xml added.

Goes in django/tests/regressiontests/fixtures_regress/fixtures

11/12/07 19:03:30 changed by keithb

  • needs_better_patch deleted.

Russell, I believe the latest version has addressed your concerns. Apologies for the long delay in getting back to this bug.

12/31/07 00:18:29 changed by jshaffer

  • cc changed from jshaffer2112@gmail.com, kbussell@gmail.com to kbussell@gmail.com.

06/08/08 03:21:18 changed by russellm

  • status changed from new to closed.
  • resolution set to fixed.

(In [7595]) Fixed #4371 -- Improved error checking when loading fixtures. Code now catches explicitly named fixture formats that are not supported (e.g, YAML fixtures if you don't have PyYAML installed), and fixtures that are empty (which can happen due to XML tag errors). Thanks to John Shaffer for the suggestion, and Keith Bussell for his work on the fix.


Add/Change #4371 (fixture loading fails silently in testcases)




Change Properties
Action