Code review comment for lp:~abompard/mailman/failing-test

Revision history for this message
Barry Warsaw (barry) wrote :

Thanks! I made a few small changes.

* in TestSchemaManager.setUp(), I used md.sorted_tables so I didn't have to call .values():

        for table in md.sorted_tables:
            table.drop(config.db.engine)

* In test_initial() I used assertIn() on md.tables (since if it's a mapping, `in` should automatically compare against .keys()):

        self.assertIn('alembic_version', md.tables)

I don't quite understand the code though, and I would like to add some comments to the test.

* After calling setup_database(), why would you expect that alembic.command.upgrade was not called?

* After calling setup_database(), would you expect alembic_version table to exist, or would it only exist after the md.reflect() call?

review: Approve

« Back to merge proposal