#22918 closed Bug (fixed)
migrations.operations.SeparateDatabaseAndState crashes (and has no tests)
Reported by: | Tim Graham | 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 by , 10 years ago
Has patch: | set |
---|---|
Needs tests: | set |
Severity: | Normal → Release blocker |
Summary: | Add tests for db.migrations.operations.SeparateDatabaseAndState → migrations.operations.SeparateDatabaseAndState crashes (and has no tests) |
Version: | master → 1.7-rc-3 |
comment:2 by , 10 years ago
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:5 by , 10 years ago
Severity: | Release blocker → Normal |
---|
Demoting to Normal as the feature is no longer documented and thus not public API.
comment:6 by , 10 years ago
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 by , 10 years ago
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.
comment:8 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Promoting to release blocker as it seems the code doesn't work as described in this PR.