Opened 13 months ago

Closed 11 months ago

Last modified 11 months ago

#22918 closed Bug (fixed)

migrations.operations.SeparateDatabaseAndState crashes (and has no tests)

Reported by: timo Owned by: nobody
Component: Migrations Version: 1.7-rc-3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Added in [bacbbb48] and coverage shows that none of the lines are tested.

Change History (11)

comment:1 Changed 11 months ago by timgraham

  • Has patch set
  • Needs tests set
  • Severity changed from Normal to Release blocker
  • Summary changed from Add tests for db.migrations.operations.SeparateDatabaseAndState to migrations.operations.SeparateDatabaseAndState crashes (and has no tests)
  • Version changed from master to 1.7-rc-3

Promoting to release blocker as it seems the code doesn't work as described in this PR.

comment:2 Changed 11 months ago by andrewgodwin

If there's no other RBs then we can just remove the documentation from it and demote it as a feature. If there are we can pull in the patch.

comment:3 Changed 11 months ago by Andrew Godwin <andrew@…>

In a8521a2c228bd9e981dc8a2bea4e26f4544a52a7:

Undocumented SeparateDatabaseAndState so crash is not RB. Refs #22918

comment:4 Changed 11 months ago by Andrew Godwin <andrew@…>

In a7ac5f018726694e3a79180ef97f2813c715fac0:

[1.7.x] Undocumented SeparateDatabaseAndState so crash is not RB. Refs #22918

comment:5 Changed 11 months ago by andrewgodwin

  • Severity changed from Release blocker to Normal

Demoting to Normal as the feature is no longer documented and thus not public API.

comment:6 Changed 11 months ago by etianen

I think this should really count as a release blocker. The code is trivially broken, and trivially fixable. Removing it from the docs just sidesteps the issue, and there are some migration operations that can only be done using this class. (So far as I can tell, this is the only way to move models from one app to another. Maybe there is another, simpler way? If so, I'm all ears!)

I'll spend a couple of hours today writing the tests. Really, though, if the patch can't be pulled because of lack of tests, how did the original feature get in with no tests? I'm 100% for maintaining code quality, but if the core developers are not subject to the same code quality guidelines, an insistence on tests just makes it harder for non-core developers to contribute.

comment:7 Changed 11 months ago by etianen

  • Needs documentation set
  • Needs tests unset

I've created a new pull request that fixes the crash and adds in the missing tests:

https://github.com/django/django/pull/3149

I'd imagine that this would also require [a7ac5f018726694e3a79180ef97f2813c715fac0] and [a8521a2c228bd9e981dc8a2bea4e26f4544a52a7] to be reverted.

Last edited 11 months ago by etianen (previous) (diff)

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

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

In e03b7940e564a6a88269b3030e43354baabcd669:

Fixed #22918 -- Fixed SeparateDatabaseAndState crash

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

In e9264bc25d265ba1c8ff69517dea8c5ce6ad6df8:

Revert "Undocumented SeparateDatabaseAndState so crash is not RB. Refs #22918"

This reverts commit a8521a2c228bd9e981dc8a2bea4e26f4544a52a7.

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

In 7eabd22217dcf2a67173b055ace63ca3fce8eb48:

[1.7.x] Fixed #22918 -- Fixed SeparateDatabaseAndState crash

Backport of e03b7940e5 from master

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

In 52eadbf5c0bad53ca46eea3bb96a17c3320b55b2:

Revert "[1.7.x] Undocumented SeparateDatabaseAndState so crash is not RB. Refs #22918"

This reverts commit a7ac5f018726694e3a79180ef97f2813c715fac0.

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