Opened 3 years ago

Closed 3 years ago

#33022 closed Bug (fixed)

main-random test failures (build #8): field_deconstruction.tests.FieldDeconstructionTests and migrations.test_commands.MigrateTests

Reported by: Chris Jerdonek Owned by: Mariusz Felisiak
Component: Migrations Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Chris Jerdonek)

I noticed that main-random job #8 failed recently. I don't know if these were "real" failures. The failures were (database=spatialite,bionic,python3.9)--

  • field_deconstruction.tests.FieldDeconstructionTests.test_foreign_key
  • field_deconstruction.tests.FieldDeconstructionTests.test_many_to_many_field
  • field_deconstruction.tests.FieldDeconstructionTests.test_one_to_one
  • migrations.test_commands.MigrateTests.test_migrate_partially_applied_squashed_migration
  • migrations.test_commands.MigrateTests.test_migrate_plan

The shuffle seed was 5300426296.

These recent PR's seem related: PR #14730 (ticket #32983) for the FieldDeconstructionTests ones and PR #14727 (ticket #29063) for the MigrateTests ones.

Change History (13)

comment:1 by Chris Jerdonek, 3 years ago

I narrowed down the MigrateTests failures somewhat to--

./tests/runtests.py --shuffle 5300426296 migrations.test_commands.MigrateTests migrations.test_executor.ExecutorTests
...
Ran 59 tests in 2.041s
FAILED (errors=2)
Last edited 3 years ago by Chris Jerdonek (previous) (diff)

comment:2 by Chris Jerdonek, 3 years ago

Here is a shuffle seed of the same migration tests as above, but with 4 failures:

./tests/runtests.py migrations.test_commands.MigrateTests migrations.test_executor.ExecutorTests --shuffle 3472950421
...
ERROR: test_migrate_partially_applied_squashed_migration (migrations.test_commands.MigrateTests)
ERROR: test_migrate_fake_split_initial (migrations.test_commands.MigrateTests)
ERROR: test_sqlmigrate_backwards (migrations.test_commands.MigrateTests)
FAIL: test_migrate (migrations.test_commands.MigrateTests)
...
Last edited 3 years ago by Chris Jerdonek (previous) (diff)

comment:3 by Chris Jerdonek, 3 years ago

Description: modified (diff)

comment:4 by Carlton Gibson, 3 years ago

Triage Stage: UnreviewedAccepted

Yep. Nice work. Thanks!

comment:5 by Carlton Gibson, 3 years ago

Component: UncategorizedMigrations
Type: Cleanup/optimizationBug

comment:6 by Carlton Gibson, 3 years ago

Severity: NormalRelease blocker

comment:7 by Jacob Walls, 3 years ago

Has patch: set

Thanks, Chris. For the migration tests, I've narrowed down the pair of problematic tests. Just not 100% certain yet which one needs to be adjusted until I understand the test I didn't author a bit more. Any advice is welcome.

PR

comment:8 by Mariusz Felisiak, 3 years ago

Has patch: unset
Severity: Release blockerNormal

It's a test isolation issue not a regression.

comment:9 by Jacob Walls, 3 years ago

Reiterating from above PR:

The pair of migrations tests that produces a failure is:

  • test_custom_user
  • test_migrate_partially_applied_squashed_migration

Can be produced with:

./tests/runtests.py migrations.test_commands.MigrateTests migrations.test_executor.ExecutorTests -k test_custom_user -k test_migrate_partially --shuffle 1022528553

The failure occurs during the migrate (to zero) command in the finally block with:

    raise ValueError("\n".join(error.msg for error in errors))
ValueError: The field migrations.Book.author was declared with a lazy reference to 'auth.user', but app 'auth' isn't installed.

test_custom_user overrides the AUTH_USER_MODEL setting like so:

        AUTH_USER_MODEL="migrations.Author",

Documentation for the setting says that it's cumbersome to change this after migrations have been made:

Changing AUTH_USER_MODEL after you’ve created database tables is significantly more difficult since it affects foreign keys and many-to-many relationships, for example. This change can’t be done automatically and requires manually fixing your schema, moving your data from the old user table, and possibly manually reapplying some migrations. See #25313 for an outline of the steps.

This makes sense, because in the ordinary test execution order, the squashed migration in test_migrate_partially_applied_squashed_migration creates a Book.Author field like this:

('author', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to='migrations.author')),

But in the failing test order, instead creates this:

('author', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, to=settings.AUTH_USER_MODEL)),

Since the documentation mentions this is cumbersome to address, the thought in the PR was just to keep all tests running on the app "migrations" with the same AUTH_USER_MODEL, but if that's not satisfactory, is the idea that we should rewrite test_migrate_partially_applied_squashed_migration to use a different app to run the test cases? Otherwise I'm not sure how to prevent squashmigrations from picking up the swappable user model.

Thanks, and happy to keep digging if helpful.

comment:10 by Chris Jerdonek, 3 years ago

Can be produced with:

FYI, once you have specific tests to reproduce with, you can dispense with the --shuffle argument and just provide the order manually (not needing -k, etc):

./tests/runtests.py migrations.test_executor.ExecutorTests.test_custom_user migrations.test_commands.MigrateTests.test_migrate_partially_applied_squashed_migration

comment:11 by Mariusz Felisiak, 3 years ago

Has patch: set
Owner: changed from nobody to Mariusz Felisiak
Status: newassigned

comment:12 by Jacob Walls, 3 years ago

Triage Stage: AcceptedReady for checkin

Verified the FieldDeconstruction test isolation issues also were resolved (using the original shuffle seed).

comment:13 by GitHub <noreply@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 01bf679e:

Fixed #33022 -- Fixed isolation of migrations.test_executor.ExecutorTests.test_custom_user().

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