launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09152
[Merge] lp:~jtv/maas/make-name into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/make-name into lp:maas.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/make-name/+merge/111800
Don't let the size of this diff put you off; it's mostly air. I'm hoping to resolve a long-standing controversy in test styles.
One style uses hard-coded strings as test data. These are often either predictable, reducing effective test coverage and leaving open forgotten subtleties; or so tedious that engineers pick confusingly arbitrary names from unrelated domains.
The other style uses factory-generated random strings. But these are inherently difficult to recognize and trace back to source code while debugging.
And so I'd like to introduce to you a new compromise method: factory.make_name(). You pass it a prefix (and optionally a separator if the default dash is not suitable) and it returns a name starting with the prefix and the separator, but ending in a randomized string. If you want to produce a node name, for example, factory.make_name('node') will give you a string like “node-aX6o0iL” and you sort of get the benefit of both testing styles.
The name of the new method does not follow the style of factory.getRandomString and cousins. As I recall the standing choice for MAAS was to use PEP-8 naming. I brought this up on IRC twice, and I know people have read it but other things always took precedence. I'll take that as it not being worth squabbling over.
Nor did I choose to add optional prefix and separator parameters to getRandomString. That would have been too long, for one, especially when the new parameter is so far back in the list that you have to specify it by name. Plus it would have raised awkward questions of semantics for the size parameter — in a dedicated method I can feel I can get away with more.
A good portion of the diff you see is, of course, from trivial conversions of existing practice. I didn't go overboard with introducing prefixes everywhere, just replaced similar things we were doing already.
Jeroen
--
https://code.launchpad.net/~jtv/maas/make-name/+merge/111800
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/make-name into lp:maas.
=== modified file 'src/maas/tests/test_maas_import_pxe_files.py'
--- src/maas/tests/test_maas_import_pxe_files.py 2012-06-21 09:23:16 +0000
+++ src/maas/tests/test_maas_import_pxe_files.py 2012-06-25 09:15:32 +0000
@@ -29,11 +29,6 @@
)
-def make_name(prefix, separator='-'):
- """Return an arbitrary name, with the given prefix."""
- return separator.join([prefix, factory.getRandomString(4)])
-
-
def read_file(path, name):
"""Return the contents of the file at `path/name`."""
with open(os.path.join(path, name)) as infile:
@@ -78,9 +73,9 @@
Returns an "ARCHIVE" URL for the download.
"""
if release is None:
- release = make_name('release')
+ release = factory.make_name('release')
if arch is None:
- arch = make_name('arch')
+ arch = factory.make_name('arch')
archive = self.make_dir()
download = compose_download_dir(archive, arch, release)
os.makedirs(download)
@@ -118,7 +113,7 @@
check_call(script, env=env, stdout=dev_null)
def test_downloads_pre_boot_loader(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
archive = self.make_downloads(arch=arch, release=release)
tftproot = self.make_dir()
@@ -129,7 +124,7 @@
self.assertThat(tftp_path, FileContains(expected_contents))
def test_ignores_missing_pre_boot_loader(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
archive = self.make_downloads(arch=arch, release=release)
download_path = compose_download_dir(archive, arch, release)
@@ -140,7 +135,7 @@
self.assertThat(tftp_path, Not(FileExists()))
def test_updates_pre_boot_loader(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
tftproot = self.make_dir()
tftp_path = compose_tftp_path(tftproot, arch, 'pxelinux.0')
@@ -156,7 +151,7 @@
def test_leaves_pre_boot_loader_untouched_if_unchanged(self):
# If pxelinux.0 has not changed between script runs, the old
# copy stays in place.
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
archive = self.make_downloads(arch=arch, release=release)
tftproot = self.make_dir()
@@ -168,7 +163,7 @@
self.assertEqual(original_timestamp, get_write_time(tftp_path))
def test_downloads_install_image(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
archive = self.make_downloads(arch=arch, release=release)
tftproot = self.make_dir()
@@ -180,7 +175,7 @@
self.assertThat(tftp_path, FileContains(expected_contents))
def test_updates_install_image(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
tftproot = self.make_dir()
tftp_path = compose_tftp_path(
@@ -195,7 +190,7 @@
self.assertThat(tftp_path, FileContains(expected_contents))
def test_leaves_install_image_untouched_if_unchanged(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
archive = self.make_downloads(arch=arch, release=release)
tftproot = self.make_dir()
@@ -208,7 +203,7 @@
self.assertEqual(original_timestamp, get_write_time(tftp_path))
def test_generates_default_pxe_config(self):
- arch = make_name('arch')
+ arch = factory.make_name('arch')
release = 'precise'
tftproot = self.make_dir()
archive = self.make_downloads(arch=arch, release=release)
=== modified file 'src/maasserver/tests/test_api_mechanism.py'
--- src/maasserver/tests/test_api_mechanism.py 2012-04-30 07:31:02 +0000
+++ src/maasserver/tests/test_api_mechanism.py 2012-06-25 09:15:32 +0000
@@ -14,13 +14,12 @@
__metaclass__ = type
__all__ = []
-from maastesting.testcase import TestCase
-from maasserver.testing.factory import factory
-
from maasserver.api import (
api_exported,
dispatch_methods,
)
+from maasserver.testing.factory import factory
+from maastesting.testcase import TestCase
class TestApiExported(TestCase):
@@ -29,7 +28,7 @@
def test_invalid_method(self):
# If the supplied HTTP method is not in the allowed set, it should
# raise a ValueError.
- random_method = "method" + factory.getRandomString(4)
+ random_method = factory.make_name('method', sep='')
decorate = api_exported(random_method)
self.assertRaises(ValueError, decorate, lambda: None)
@@ -58,9 +57,8 @@
def test_can_pass_export_as(self):
# Test that passing the optional "export_as" works as expected.
- random_exported_name = "exportedas" + factory.getRandomString()
- decorate = api_exported(
- "POST", exported_as=random_exported_name)
+ random_exported_name = factory.make_name("exportedas", sep='')
+ decorate = api_exported("POST", exported_as=random_exported_name)
decorated = decorate(lambda: None)
self.assertEqual(
=== modified file 'src/maasserver/tests/test_commands_install_pxe_image.py'
--- src/maasserver/tests/test_commands_install_pxe_image.py 2012-06-25 07:50:28 +0000
+++ src/maasserver/tests/test_commands_install_pxe_image.py 2012-06-25 09:15:32 +0000
@@ -30,21 +30,13 @@
)
-def make_random_string(prefix):
- """Return an arbitrary string starting with the given prefix."""
- return '-'.join([prefix, factory.getRandomString(5)])
-
-
def make_arch_subarch_release():
"""Create arbitrary architecture/subarchitecture/release names.
:return: A triplet of three identifiers for these respective items.
"""
- return (
- make_random_string('arch'),
- make_random_string('subarch'),
- make_random_string('release'),
- )
+ return tuple(
+ factory.make_name(item) for item in ('arch', 'subarch', 'release'))
class TestInstallPXEImage(TestCase):
=== modified file 'src/maasserver/tests/test_filestorage.py'
--- src/maasserver/tests/test_filestorage.py 2012-06-21 09:23:16 +0000
+++ src/maasserver/tests/test_filestorage.py 2012-06-25 09:15:32 +0000
@@ -122,7 +122,7 @@
# reference to the old data gets overwritten with one to the new
# data. They are actually different files on the filesystem.
self.make_upload_dir()
- filename = 'filename-%s' % factory.getRandomString()
+ filename = factory.make_name('filename')
old_storage = factory.make_file_storage(
filename=filename, data=self.make_data('old data'))
new_data = self.make_data('new-data')
=== modified file 'src/maasserver/tests/test_provisioning.py'
--- src/maasserver/tests/test_provisioning.py 2012-06-22 09:48:17 +0000
+++ src/maasserver/tests/test_provisioning.py 2012-06-25 09:15:32 +0000
@@ -259,7 +259,7 @@
def make_node_without_saving(self, arch=ARCHITECTURE.i386):
"""Create a Node, but don't save it to the database."""
- system_id = "node-%s" % factory.getRandomString()
+ system_id = factory.make_name("node")
return Node(
system_id=system_id, hostname=factory.getRandomString(),
status=NODE_STATUS.DEFAULT_STATUS, after_commissioning_action=(
=== modified file 'src/maasserver/tests/test_views_settings.py'
--- src/maasserver/tests/test_views_settings.py 2012-04-23 14:02:51 +0000
+++ src/maasserver/tests/test_views_settings.py 2012-06-25 09:15:32 +0000
@@ -212,10 +212,10 @@
user = factory.make_user()
params = make_user_attribute_params(user)
params.update({
- 'last_name': 'Newname-%s' % factory.getRandomString(),
+ 'last_name': factory.make_name('Newname'),
'email': 'new-%s@xxxxxxxxxxx' % factory.getRandomString(),
'is_superuser': True,
- 'username': 'newname-%s' % factory.getRandomString(),
+ 'username': factory.make_name('newname'),
})
response = self.client.post(
=== modified file 'src/maastesting/factory.py'
--- src/maastesting/factory.py 2012-04-25 13:00:38 +0000
+++ src/maastesting/factory.py 2012-06-25 09:15:32 +0000
@@ -97,6 +97,23 @@
f.write(contents)
return path
+ def make_name(self, prefix=None, sep='-', size=6):
+ """Generate a random name.
+
+ :param prefix: Optional prefix. Pass one to help make test failures
+ and tracebacks easier to read! If you don't, you might as well
+ use `getRandomString`.
+ :param sep: Separator that will go between the prefix and the random
+ portion of the name. Defaults to a dash.
+ :param size: Length of the random portion of the name. Don't get
+ hung up on this; you may need more if uniqueness is really
+ important or less if it doesn't but legibility does, but
+ generally, use the default.
+ :return: A randomized unicode string.
+ """
+ return sep.join(
+ filter(None, [prefix, self.getRandomString(size=size)]))
+
# Create factory singleton.
factory = Factory()
=== modified file 'src/maastesting/tests/test_factory.py'
--- src/maastesting/tests/test_factory.py 2012-04-25 12:53:19 +0000
+++ src/maastesting/tests/test_factory.py 2012-06-25 09:15:32 +0000
@@ -14,12 +14,17 @@
from datetime import datetime
import os.path
+from random import randint
from maastesting.factory import factory
from maastesting.testcase import TestCase
from testtools.matchers import (
+ Contains,
FileContains,
FileExists,
+ MatchesAll,
+ Not,
+ StartsWith,
)
@@ -72,3 +77,33 @@
self.assertEqual(
(directory, name),
os.path.split(factory.make_file(directory, name=name)))
+
+ def test_make_name_returns_unicode(self):
+ self.assertIsInstance(factory.make_name(), unicode)
+
+ def test_make_name_combines_prefix_sep_and_random_text(self):
+ self.assertThat(factory.make_name('abc'), StartsWith('abc-'))
+
+ def test_make_name_includes_random_text_of_requested_length(self):
+ size = randint(1, 99)
+ self.assertEqual(
+ len('prefix') + len('-') + size,
+ len(factory.make_name('prefix', size=size)))
+
+ def test_make_name_uses_configurable_separator(self):
+ sep = ':%s:' % factory.getRandomString(3)
+ prefix = factory.getRandomString(3)
+ self.assertThat(
+ factory.make_name(prefix, sep=sep),
+ StartsWith(prefix + sep))
+
+ def test_make_name_does_not_require_prefix(self):
+ size = randint(1, 99)
+ unprefixed_name = factory.make_name(sep='-', size=size)
+ self.assertEqual(size, len(unprefixed_name))
+ self.assertThat(unprefixed_name, Not(StartsWith('-')))
+
+ def test_make_name_does_not_include_weird_characters(self):
+ self.assertThat(
+ factory.make_name(size=100),
+ MatchesAll(*[Not(Contains(char)) for char in '/ \t\n\r\\']))
=== modified file 'src/provisioningserver/power/tests/test_poweraction.py'
--- src/provisioningserver/power/tests/test_poweraction.py 2012-06-14 05:52:58 +0000
+++ src/provisioningserver/power/tests/test_poweraction.py 2012-06-25 09:15:32 +0000
@@ -36,7 +36,7 @@
"""Tests for PowerAction."""
def test_init_raises_for_unknown_powertype(self):
- powertype = "powertype" + factory.getRandomString()
+ powertype = factory.make_name("powertype", sep='')
self.assertRaises(
UnknownPowerType,
PowerAction, powertype)
