launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/test-for-duplicate-migrations into lp:maas.

Commit message:
Test against duplicate sequence numbers in South database migrations (such as we recently noticed for #2 and #39 in maasserver).

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/test-for-duplicate-migrations/+merge/132519

This branch can't be landed until the problem it checks for is resolved in our source tree.

Clashes in migration numbering will be reported as a failure on “test_migrations_have_unique_numbers.” The failure output lists, in order, each of the clashing sequence numbers and migration filenames.  For full output of how it reports the problem we have in the tree right now, see http://paste.ubuntu.com/1323236/

The little test case needs to be repeated in every MAAS app that uses South migrations — currently only maasserver and metadataserver.  I made sure the repetition was mostly boilerplate.  The difference between instances of the test case is hidden within the magic of “__file__”: each leads the generic code to its own app's migrations directory.

I'm sure there would be ways of eliminating the repetition altogether, maybe by softlinking the duplicate tests or by creating a reusable TestCase (and preventing Nose from running it in maastesting), but I strove to maintain some isolation between apps.  It'd be nasty to have one app peek into another's source tree, let alone to have one app link into another's source tree.  My choice would be worth reconsidering if we add further tests, but I don't think it's a problem just yet.  For now I just documented the situation.


Jeroen
-- 
https://code.launchpad.net/~jtv/maas/test-for-duplicate-migrations/+merge/132519
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/test-for-duplicate-migrations into lp:maas.

=== added file 'src/maasserver/tests/test_migrations.py'
--- src/maasserver/tests/test_migrations.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/tests/test_migrations.py	2012-11-01 11:32:21 +0000
@@ -0,0 +1,30 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Sanity checks for database migrations.
+
+These tests need to be included in each of the MAAS applications that has
+South-managed database migrations.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maastesting.db_migrations import (
+    detect_sequence_clashes,
+    locate_migrations,
+    )
+from maastesting.testcase import TestCase
+
+
+class TestMigrations(TestCase):
+
+    def test_migrations_have_unique_numbers(self):
+        migrations_dir = locate_migrations(__file__)
+        self.assertEqual([], detect_sequence_clashes(migrations_dir))

=== added file 'src/maastesting/db_migrations.py'
--- src/maastesting/db_migrations.py	1970-01-01 00:00:00 +0000
+++ src/maastesting/db_migrations.py	2012-11-01 11:32:21 +0000
@@ -0,0 +1,82 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helpers for testing South database migrations.
+
+Each Django application in MAAS tests the basic sanity of its own South
+database migrations.  To minimize repetition, this single module provides all
+the code those tests need.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'detect_sequence_clashes',
+    'locate_migrations',
+    ]
+
+from collections import Counter
+import os
+import re
+
+
+def locate_migrations(test_module):
+    """Find the migrations dir for the application that `test_module` is in.
+
+    :param test_module: The `__file__` of a test that checks the migrations
+        for a particular MAAS sub-application.
+    :return: Path to the MAAS sub-application's South migrations.
+    """
+    test_dir = os.path.dirname(test_module)
+    application_dir = os.path.dirname(test_dir)
+    return os.path.join(application_dir, 'migrations')
+
+
+def match_migration_name(filename):
+    """Return a regex match on a migration filename; None if no match.
+
+    The match, if any, has the migration's sequence number as its first group.
+    """
+    return re.match('([0-9]+)_[^.]+\\.py$', filename)
+
+
+def extract_number(migration_name):
+    """Extract the sequence number from a migration module name."""
+    return int(match_migration_name(migration_name).groups()[0])
+
+
+def get_duplicates(numbers):
+    """Return set of those items that occur more than once."""
+    return {
+        numbers
+        for numbers, count in Counter(numbers).items()
+            if count > 1}
+
+
+def list_migrations(migrations_dir):
+    """List schema-migration files in `migrations_dir`."""
+    return filter(
+        lambda name: match_migration_name(name) is not None,
+        os.listdir(migrations_dir))
+
+
+def detect_sequence_clashes(migrations_dir):
+    """List numbering clashes among database migrations in given directory.
+
+    :param migrations_dir: Location of an application's migration modules.
+    :return: A sorted `list` of tuples `(number, filename)` representing all
+        migration modules in `migrations_dir`.  The `number` is as found in
+        `filename`, but in `int` form.
+    """
+    migrations = list_migrations(migrations_dir)
+    numbers_and_names = [(extract_number(name), name) for name in migrations]
+    duplicates = get_duplicates(number for number, name in numbers_and_names)
+    return sorted(
+        (number, name)
+        for number, name in numbers_and_names
+            if number in duplicates)

=== added file 'src/maastesting/tests/test_db_migrations.py'
--- src/maastesting/tests/test_db_migrations.py	1970-01-01 00:00:00 +0000
+++ src/maastesting/tests/test_db_migrations.py	2012-11-01 11:32:21 +0000
@@ -0,0 +1,125 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for helpers used to sanity-check South migrations."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+import os.path
+from random import randint
+
+from maastesting.db_migrations import (
+    detect_sequence_clashes,
+    extract_number,
+    get_duplicates,
+    list_migrations,
+    locate_migrations,
+    match_migration_name,
+    )
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+
+
+def make_migration_name(number=None, name=None):
+    """Create a migration-file name."""
+    if number is None:
+        number = randint(0, 9999)
+    if name is None:
+        name = factory.getRandomString()
+    return '{0:=04}_{1}.py'.format(number, name)
+
+
+class TestDBMigrations(TestCase):
+
+    def test_locate_migrations_finds_migrations_dir_from_test(self):
+        app_dir = os.path.join(
+            factory.make_name('branch'), 'src', factory.make_name('app'))
+        test_name = 'test_%s.py' % factory.make_name()
+        test = os.path.join(app_dir, 'tests', test_name)
+
+        self.assertEqual(
+            os.path.join(app_dir, 'migrations'),
+            locate_migrations(test))
+
+    def test_match_migration_name_matches_real_migration_name(self):
+        self.assertIsNotNone(match_migration_name('0002_macaddress_unique.py'))
+
+    def test_match_migration_name_matches_any_migration_name(self):
+        self.assertIsNotNone(match_migration_name(make_migration_name()))
+
+    def test_match_migration_name_ignores_other_files(self):
+        self.assertIsNone(match_migration_name('__init__.py'))
+
+    def test_match_migration_name_ignores_unnumbered_file(self):
+        self.assertIsNone(match_migration_name('macaddress_unique.py'))
+
+    def test_match_migration_name_ignores_non_python_file(self):
+        self.assertIsNone(match_migration_name('0002_macaddress_unique'))
+
+    def test_match_migration_name_ignores_dot_files(self):
+        self.assertIsNone(match_migration_name('.' + make_migration_name()))
+
+    def test_match_migration_name_ignores_suffixed_names(self):
+        self.assertIsNone(match_migration_name(make_migration_name() + '~'))
+
+    def test_extract_number_returns_sequence_number(self):
+        number = randint(0, 999999)
+        self.assertEqual(number, extract_number(make_migration_name(number)))
+
+    def test_get_duplicates_finds_duplicates(self):
+        item = factory.make_name('item')
+        self.assertEqual({item}, get_duplicates([item, item]))
+
+    def test_get_duplicates_finds_all_duplicates(self):
+        dup1 = factory.make_name('dup1')
+        dup2 = factory.make_name('dup2')
+        self.assertEqual({dup1, dup2}, get_duplicates(2 * [dup1, dup2]))
+
+    def test_get_duplicates_ignores_unique_items(self):
+        self.assertEqual(set(), get_duplicates(range(5)))
+
+    def test_get_duplicates_ignores_ordering(self):
+        dup = factory.make_name('dup')
+        unique = factory.make_name('unique')
+        self.assertEqual({dup}, get_duplicates([dup, unique, dup]))
+
+    def test_list_migrations_does_not_include_path(self):
+        migration = make_migration_name()
+        self.assertItemsEqual(
+            [migration],
+            list_migrations(os.path.dirname(self.make_file(migration))))
+
+    def test_list_migrations_lists_all_migrations_and_only_migrations(self):
+        migrations = [make_migration_name(number) for number in range(3)]
+        location = self.make_dir()
+        for migration in migrations:
+            factory.make_file(location, migration)
+        for other_file in ['__init__.py', '__init__.pyc', 'README.txt']:
+            factory.make_file(location, other_file)
+        self.assertItemsEqual(migrations, list_migrations(location))
+
+    def test_detect_sequence_clashes_returns_list(self):
+        self.assertEqual([], detect_sequence_clashes(self.make_dir()))
+
+    def test_detect_sequence_clashes_finds_clashes(self):
+        location = self.make_dir()
+        number = randint(0, 999)
+        names = [make_migration_name(number) for counter in range(2)]
+        for name in names:
+            factory.make_file(location, name)
+        self.assertItemsEqual(
+            [(number, name) for name in names],
+            detect_sequence_clashes(location))
+
+    def test_detect_sequence_clashes_ignores_unique_migrations(self):
+        location = self.make_dir()
+        for number in range(5):
+            factory.make_file(location, make_migration_name(number))
+        self.assertItemsEqual([], detect_sequence_clashes(location))

=== added file 'src/metadataserver/tests/test_migrations.py'
--- src/metadataserver/tests/test_migrations.py	1970-01-01 00:00:00 +0000
+++ src/metadataserver/tests/test_migrations.py	2012-11-01 11:32:21 +0000
@@ -0,0 +1,30 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Sanity checks for database migrations.
+
+These tests need to be included in each of the MAAS applications that has
+South-managed database migrations.
+"""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from maastesting.db_migrations import (
+    detect_sequence_clashes,
+    locate_migrations,
+    )
+from maastesting.testcase import TestCase
+
+
+class TestMigrations(TestCase):
+
+    def test_migrations_have_unique_numbers(self):
+        migrations_dir = locate_migrations(__file__)
+        self.assertEqual([], detect_sequence_clashes(migrations_dir))