Support both successful and failing migrations in the migration test utility #1267
Conversation
350eba8 to
bf80bb2
Compare
| self.revert_to_migrate_from() | ||
| self.setUpBeforeMigration(self.old_apps) | ||
| self.apply_migration_to() | ||
| self.restore_constraint_checks() |
There was a problem hiding this comment.
The level of abstraction strikes me as uneven across the class. Restoration of constraint checks is abstracted in the method restore_constraint_checks, but the original change ("SET CONSTRAINTS ALL IMMEDIATE") is implicit in setUpBeforeMigration. Loooking just at this code, I can see immediately that constraints are being restored, but I don't see why that's happening because the original change is hidden. If possible (perhaps there are complications), I'd prefer having both of these implicit or both explicit in a separate method.
There was a problem hiding this comment.
I agree. I've made two changes:
- Explicitly marked the boilerplate method as private'ish
- Refactored the constraint management into a contextmanager, applied only to the migrate calls.
|
|
||
| def setUp(self): | ||
| self.revert_to_migrate_from() | ||
| self.setUpBeforeMigration(self.old_apps) |
There was a problem hiding this comment.
setUpBeforeMigration has cursor.execute("SET CONSTRAINTS ALL IMMEDIATE"), which is executed independently of whether the migration succeeds. Do we not need to restore the constraints?
There was a problem hiding this comment.
The deferred constraints are what the database runs with by default, so I do think it's important that they're restored in all codepaths to avoid flaky tests that might depend on this behavior. I believe the next contextmanager should take care of that.
bf80bb2 to
cf03b3d
Compare
|
For posterity: we attempt another refactor (using a context manager rather than a testcase) to get better "Arrange, Act, Assert" semantics, but that approach traded readability for poorer separation (pre- and post-migration symbols living in the same test scope). We're sticking with this for now, and might return to an improved API at a later point. |
I've had to write a few non-trivial migrations for the multi-ZGW backend work, and I suspect there will be a few more. To keep things consistent and readable, I've expanded the migration testing utility to support both successful and failing migrations.