Merge lp:~abompard/mailman/failing-test into lp:mailman

[Aurélien Bompard]

Fix a failing schemamanager test

[Barry Warsaw (barry)]

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

[Aurélien Bompard (abompard)]

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

Because when there is no existing database, I'm not using alembic.command.upgrade, I'm creating all tables in SQLAlchemy with Metadata.create_all and them I'm stamping the schema at Alembic's latest version with alembic.command.stamp. So I'm checking that I'm not going through a different code path.

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

The table is created by the Alembic stamp command, but it's not in Mailman's metadata (we have no model for it since we don't use it), so I have to detect its presence using md.reflect().

Does this answer your questions?

[Barry Warsaw (barry)]

On Nov 29, 2014, at 08:38 AM, Aurélien Bompard wrote:

>Does this answer your questions?

It does, thanks! I will update the comments.

=== modified file 'src/mailman/database/tests/test_factory.py'
--- src/mailman/database/tests/test_factory.py	2014-10-13 19:24:24 +0000
+++ src/mailman/database/tests/test_factory.py	2014-11-27 13:31:21 +0000
@@ -51,9 +51,9 @@
         Model.metadata.drop_all(config.db.engine)
         md = MetaData()
         md.reflect(bind=config.db.engine)
-        for tablename in ('alembic_version', 'version'):
-            if tablename in md.tables:
-                md.tables[tablename].drop(config.db.engine)
+        # Drop leftover tables (alembic & storm schema versions)
+        for table in md.tables.values():
+            table.drop(config.db.engine)
         self.schema_mgr = SchemaManager(config.db)
 
     def tearDown(self):
@@ -114,15 +114,20 @@
             self.assertFalse(alembic_command.stamp.called)
             self.assertFalse(alembic_command.upgrade.called)
 
-    @patch('alembic.command')
-    def test_initial(self, alembic_command):
+    @patch('alembic.command.upgrade')
+    def test_initial(self, alembic_command_upgrade):
         # No existing database.
         self.assertFalse(self._table_exists('mailinglist'))
         self.assertFalse(self._table_exists('alembic_version'))
-        self.schema_mgr.setup_database()
-        self.assertFalse(alembic_command.upgrade.called)
+        head_rev = self.schema_mgr.setup_database()
+        self.assertFalse(alembic_command_upgrade.called)
         self.assertTrue(self._table_exists('mailinglist'))
-        self.assertTrue(self._table_exists('alembic_version'))
+        md = MetaData()
+        md.reflect(bind=config.db.engine)
+        self.assertTrue('alembic_version' in md.tables.keys())
+        current_rev = config.db.engine.execute(
+            md.tables["alembic_version"].select()).scalar()
+        self.assertEqual(current_rev, head_rev)
 
     @patch('alembic.command.stamp')
     def test_storm(self, alembic_command_stamp):