Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#4371 closed (fixed)

fixture loading fails silently in testcases

Reported by: John Shaffer <jshaffer2112@…> Owned by: keithb
Component: Testing framework Version: master
Severity: Keywords: fixtures sprintsept14
Cc: kbussell@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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

raise_verbosity.diff (473 bytes) - added by John Shaffer <jshaffer2112@…> 8 years ago.
Simply raise the verbosity level. Probably shows more messages than anyone will want.
dont_suppress_skip_messages.diff (868 bytes) - added by John Shaffer <jshaffer2112@…> 8 years ago.
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 8 years ago.
Patch for django.core.management.commands.loaddata.py
fixtures_test.diff (1.3 KB) - added by keithb 8 years ago.
Patch for tests.modeltests.fixtures
Bug4371_take2.diff (3.4 KB) - added by keithb 7 years ago.
Combined patch for fix and test
bad_fixture1.unkn (60 bytes) - added by keithb 7 years ago.
Goes in django/tests/regressiontests/fixtures_regress/fixtures
bad_fixture2.xml (300 bytes) - added by keithb 7 years ago.
Goes in django/tests/regressiontests/fixtures_regress/fixtures

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by John Shaffer <jshaffer2112@…>

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

Changed 8 years ago by John Shaffer <jshaffer2112@…>

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.

comment:1 Changed 8 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from jacob to russellm
  • Patch needs improvement unset

comment:2 Changed 8 years ago by curtis.thompson@…

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

comment:3 Changed 8 years ago by russellm

  • Component changed from Uncategorized to Unit test system
  • Triage 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.

comment:4 Changed 8 years ago by keithb

  • Owner changed from nobody to keithb

Changed 8 years ago by keithb

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

Changed 8 years ago by keithb

Patch for tests.modeltests.fixtures

comment:5 Changed 8 years ago by keithb

  • Keywords sprintsept14 added

Ready for review

comment:6 Changed 8 years ago by russellm

  • Patch needs improvement set

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.

comment:7 Changed 7 years ago by keithb

  • Cc kbussell@… added

Changed 7 years ago by keithb

Combined patch for fix and test

Changed 7 years ago by keithb

Goes in django/tests/regressiontests/fixtures_regress/fixtures

Changed 7 years ago by keithb

Goes in django/tests/regressiontests/fixtures_regress/fixtures

comment:8 Changed 7 years ago by keithb

  • Patch needs improvement unset

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

comment:9 Changed 7 years ago by jshaffer

  • Cc jshaffer2112@… removed

comment:10 Changed 7 years ago by russellm

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

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

Note: See TracTickets for help on using tickets.
Back to Top