Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tests cases for copying Docker units. #126

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pulp_smash/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
This URL can be used as the "feed" property of a Pulp Docker registry.
"""

DOCKER_V2_FEED_URL = 'https://registry-1.docker.io'
DOCKER_V2_FEED_URL = DOCKER_V1_FEED_URL
"""The URL to a V2 Docker registry.

This URL can be used as the "feed" property of a Pulp Docker registry.
Expand Down
232 changes: 232 additions & 0 deletions pulp_smash/tests/docker/cli/test_copy.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
# coding=utf-8
"""Tests for copying docker units between repositories."""
from __future__ import unicode_literals
import re

import unittest2
from packaging.version import Version

from pulp_smash import utils
from pulp_smash.tests.docker.cli import utils as docker_utils


class CopyAllImagesTestCase(docker_utils.BaseTestCase,
docker_utils.SuccessMixin):
"""Test copying all Images from one repository to another."""

@classmethod
def setUpClass(cls):
"""Create and sync a docker repository with a v1 registry."""
super(CopyAllImagesTestCase, cls).setUpClass()
if cls.cfg.version < Version('2.8'):
raise unittest2.SkipTest('These tests require Pulp 2.8 or above.')
# Create and sync a repository with a v1 feed to bring some Images in.
docker_utils.create_repo(
cls.cfg, cls.repo_id, upstream_name='library/busybox',
sync_v1=True, sync_v2=False)
docker_utils.sync_repo(cls.cfg, cls.repo_id)

# Now create a feedless repository and copy the images from the first
# repository over.
cls.copy_target = utils.uuid4()
docker_utils.create_repo(cls.cfg, cls.copy_target)
cls.completed_proc = docker_utils.copy(cls.cfg, 'image', cls.repo_id,
cls.copy_target)

@classmethod
def tearDownClass(cls):
"""Delete the copy_target repo."""
super(CopyAllImagesTestCase, cls).tearDownClass()
docker_utils.delete_repo(cls.cfg, cls.copy_target)

def test_positive_copy_output(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In just about all of the tests in Pulp Smash, communications with Pulp occur in setUpClass, and assertions occur in test methods. However, this test is structured so that communications with Pulp occur in test methods. I don't think that's ideal. Can we push these communications into setUpClass and make the test methods concern themselves just with assertions?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the benefit of refactoring it this way? This seems like a preference thing to me. The code in question is reading data that is specifically relevant to the assertion. In fact, keeping it this way has the very practical benefit of having the code next to where it is used, instead of off somewhere else. Much more readable and easier to tell what's happening when things go wrong. It's also much easier to create the test this way. What benefit is there to the alternative?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One benefit is an improved ability to re-use test methods. See an example here. In short, many of these test methods contain highly similar logic, and pushing that logic into setUpClass lets us create more reusable test methods.

A second benefit is assertion isolation. For example, given this code:

self.assertEqual(tags_in_src, tags_in_dest)
self.assertEqual(manifests_in_src, manifests_in_dest)

...if the first assertion fails, the second will not run, which is likely undesirable. Given that we have only one assertion in this test method, it does not apply in this particular case.

A third benefit is an increased ability to add or remove assertions about a particular outcome. Talking to the server and saving the munged responses just once means that we can write as many assertions as we want targeting the response(s).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your cited example, if the tags aren't in dest, the manifests won't be either. We will know there is a problem and we will fix it. I don't think that's a benefit.

Also, the general pattern in pulp-smash of performing all the "work" in setUpClass means that we need a lot more classes. This means that we sync from docker n times instead of one time in this copy module. I'm not a fan of that, and I'm sure Docker wouldn't be either. If anything, we should have one setUpClass that does the sync, and then each test should do whatever copying it needs along with whatever assertions it needs.

It's easy to add assertions to tests, so I don't think that benefit is meaningfully different between the two approaches.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your cited example, if the tags aren't in dest, the manifests won't be either.

I've seen other cases where a collection of assertions should intuitively all either succeed or fail, but did not do so. As an example, this set of assertions was added some time back:

counts = self.repo_after_sync.get('content_unit_counts', {})
self.assertEqual(counts.get('rpm'), 32)
self.assertEqual(counts.get('erratum'), 4)
self.assertEqual(counts.get('package_group'), 2)
self.assertEqual(counts.get('package_category'), 1)

A bug later cropped up that affected only the RPM counts.

Also, the general pattern in pulp-smash of performing all the "work" in setUpClass means that we need a lot more classes. This means that we sync from docker n times instead of one time in this copy module.

If repeatedly syncing from docker is problematic, there are several solutions. One is to perform a sync in setUpModule. A second is to follow the suggestion you give — and after some thought, I think the solution you give may be better, so props for bringing it up.

I'm not a fan of that, and I'm sure Docker wouldn't be either.

Is your worry that repeatedly syncing content consumes unnecessary bandwidth, thus slowing down the tests and driving up the cost of operating the docker hub?

If so: sure! As mentioned above, there are several patterns we can adopt to reduce the amount of downloading performed. In addition, #104 helpfully outlines the benefits of fetching data from local sources.

It's easy to add assertions to tests, so I don't think that benefit is meaningfully different between the two approaches.

I have found that placing business logic in setUpClass and assertions in test methods tends to make it easier to write well-isolated complex assertions and re-use test methods. Now, this is only a trend, and there are certainly classes of problems that are better solved other ways, but I would still hesitate to dismiss the benefits of this approach.

"""Assert that the list of image_ids in the copy output are correct."""
# We need to limit the fields to image_id as a hack to work around
# https://pulp.plan.io/issues/1696. Doing this has a side effect of
# showing the image_ids without splitting them with a line break
# (sigh).
units_in_src = docker_utils.search(self.cfg, 'image', self.repo_id,
fields=['image_id']).stdout
# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the image_ids. Hello regex my old
# friend, I've come to talk with you again…
image_ids_in_src = set(re.findall(r'(?:Image Id:)\s*(.*)',
units_in_src))

image_ids_printed = set(re.findall(r'(?: {2})(\w*)',
self.completed_proc.stdout))

self.assertEqual(image_ids_printed, image_ids_in_src)

def test_positive_units_copied(self):
"""Assert that all units were copied."""
units_in_src = docker_utils.search(self.cfg, 'image',
self.repo_id).stdout
units_in_dest = docker_utils.search(self.cfg, 'image',
self.copy_target).stdout

# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the unit_ids. Hello regex my old
# friend, I've come to talk with you again…
units_in_src = set(re.findall(r'(?:Unit Id:)\s*(.*)', units_in_src))
units_in_dest = set(re.findall(r'(?:Unit Id:)\s*(.*)', units_in_dest))

self.assertEqual(units_in_src, units_in_dest)

def test_task_succeeded(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This same assertion appears in all three test cases. How about adding this to SuccessMixin?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are specific to copying. We could make a copying mixin, but that seems overkill to me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a valid solution.

class _CopyMixin(object):  # pylint:disable=too-few-public-methods
    """Add assertions common to the test cases in this module."""

    def test_task_succeeded(self):
        """Assert "Copied" appears in ``self.completed_proc.stdout``."""
        self.assertIn('Copied', self.completed_proc.stdout)

"""Assert that "Copied" appears in stdout."""
self.assertIn('Copied', self.completed_proc.stdout)


class CopyAllManifestsTestCase(docker_utils.BaseTestCase,
docker_utils.SuccessMixin):
"""Test copying all Manifests from one repository to another."""

@classmethod
def setUpClass(cls):
"""Create and sync a docker repository with a v2 registry."""
super(CopyAllManifestsTestCase, cls).setUpClass()
if cls.cfg.version < Version('2.8'):
raise unittest2.SkipTest('These tests require Pulp 2.8 or above.')
# Create and sync a repository with a v2 feed to bring some Manifests
# in.
docker_utils.create_repo(
cls.cfg, cls.repo_id, upstream_name='library/busybox',
sync_v1=False, sync_v2=True)
docker_utils.sync_repo(cls.cfg, cls.repo_id)

# Now create a feedless repository and copy the manifests from the
# first repository over.
cls.copy_target = utils.uuid4()
docker_utils.create_repo(cls.cfg, cls.copy_target)
cls.completed_proc = docker_utils.copy(cls.cfg, 'manifest',
cls.repo_id, cls.copy_target)

@classmethod
def tearDownClass(cls):
"""Delete the copy_target repo."""
super(CopyAllManifestsTestCase, cls).tearDownClass()
docker_utils.delete_repo(cls.cfg, cls.copy_target)

def test_positive_copy_output(self):
"""Assert that the list of manifests in the copy output are correct."""
units_in_src = docker_utils.search(self.cfg, 'manifest', self.repo_id,
fields=['digest']).stdout
# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the digests. Hello regex my old
# friend, I've come to talk with you again…
manifest_ids_in_src = set(re.findall(r'(?:Digest:)\s*(.*)',
units_in_src))

# The manifest digests are printed after "docker_manifest:" in the copy
# output.
manifest_ids = self.completed_proc.stdout.split('docker_manifest:')[1]
manifest_ids_printed = re.findall(r'(?: {2})(\w*:\w*)', manifest_ids)
# Due to https://pulp.plan.io/issues/1696 pulp-admin has split the last
# character of each of the manifest_ids_in_src items onto the next line
# which wasn't matched by the regex. That's stupid, but we're going to
# work around it by chopping the last character off of these as well.
manifest_ids_printed = set([d[:-1] for d in manifest_ids_printed])

self.assertEqual(manifest_ids_printed, manifest_ids_in_src)

def test_positive_units_copied(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nearly identical test method is present in all three test cases. Consider adding a method like this to SuccessMixin:

def test_positive_Units_copied(self):
    """Assert all units were copied."""
    # Due to https://pulp.plan.io/issues/1693 it is not possible to get
    # pulp-admin to limit the output to the unit_ids.
    src_units = set(re.findall(r'(?:Unit Id:)\s*(.*)', self.src_units))
    dest_units = set(re.findall(r'(?:Unit Id:)\s*(.*)', self.dest_units))
    self.assertEqual(units_in_src, units_in_dest)

…and then setting self.src_units and self.dest_units in setUpClass.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They aren't actually identical, and the SuccessMixin isn't specific to copying. I don't think we need to worry about their similarities, they do work.

"""Assert that all units were copied."""
units_in_src = docker_utils.search(self.cfg, 'manifest',
self.repo_id).stdout
units_in_dest = docker_utils.search(self.cfg, 'manifest',
self.copy_target).stdout

# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the unit_ids. Hello regex my old
# friend, I've come to talk with you again…
units_in_src = set(re.findall(r'(?:Unit Id:)\s*(.*)', units_in_src))
units_in_dest = set(re.findall(r'(?:Unit Id:)\s*(.*)', units_in_dest))

self.assertEqual(units_in_src, units_in_dest)

def test_task_succeeded(self):
"""Assert that "Copied" appears in stdout."""
self.assertIn('Copied', self.completed_proc.stdout)


class CopyAllTagsTestCase(docker_utils.BaseTestCase,
docker_utils.SuccessMixin):
"""Test copying all Tags from one repository to another."""

@classmethod
def setUpClass(cls):
"""Create and sync a docker repository with a v2 registry."""
super(CopyAllTagsTestCase, cls).setUpClass()
if cls.cfg.version < Version('2.8'):
raise unittest2.SkipTest('These tests require Pulp 2.8 or above.')
# Create and sync a repository with a v2 feed to bring some Tags in.
docker_utils.create_repo(
cls.cfg, cls.repo_id, upstream_name='library/busybox',
sync_v1=False, sync_v2=True)
docker_utils.sync_repo(cls.cfg, cls.repo_id)

# Now create a feedless repository and copy the tags from the first
# repository over.
cls.copy_target = utils.uuid4()
docker_utils.create_repo(cls.cfg, cls.copy_target)
cls.completed_proc = docker_utils.copy(cls.cfg, 'tag', cls.repo_id,
cls.copy_target)

@classmethod
def tearDownClass(cls):
"""Delete the copy_target repo."""
super(CopyAllTagsTestCase, cls).tearDownClass()
docker_utils.delete_repo(cls.cfg, cls.copy_target)

def test_positive_copy_output(self):
"""Assert that the list of tags in the copy output are correct."""
units_in_src = docker_utils.search(self.cfg, 'tag', self.repo_id,
fields=['name']).stdout
# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the digests. Hello regex my old
# friend, I've come to talk with you again…
tag_ids_in_src = set(re.findall(r'(?:Digest:)\s*(.*)', units_in_src))

# The tag digests are printed after "docker_tag:" in the copy output.
tag_ids = self.completed_proc.stdout.split('docker_tag:')[1]
tag_ids_printed = re.findall(r'(?: {2})(\w*:\w*)', tag_ids)
# Due to https://pulp.plan.io/issues/1696 pulp-admin has split the last
# character of each of the tag_ids_in_src items onto the next line
# which wasn't matched by the regex. That's stupid, but we're going to
# work around it by chopping the last character off of these as well.
tag_ids_printed = set([d[:-1] for d in tag_ids_printed])

self.assertEqual(tag_ids_printed, tag_ids_in_src)

def test_positive_units_copied(self):
"""Assert that all units were copied."""
tags_in_src = docker_utils.search(self.cfg, 'tag', self.repo_id).stdout
tags_in_dest = docker_utils.search(self.cfg, 'tag',
self.copy_target).stdout
# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the names. Hello regex my old
# friend, I've come to talk with you again…
tags_in_src = set(re.findall(r'(?:Name:)\s*(.*)', tags_in_src))
tags_in_dest = set(re.findall(r'(?:Name:)\s*(.*)', tags_in_dest))

# The Manifests should have been pulled over as well since they are
# recursively copied.
manifests_in_src = docker_utils.search(self.cfg, 'manifest',
self.repo_id).stdout
manifests_in_dest = docker_utils.search(self.cfg, 'manifest',
self.copy_target).stdout
# Due to https://pulp.plan.io/issues/1693 it is not possible to get
# pulp-admin to limit the output to the unit_ids. Hello regex my old
# friend, I've come to talk with you again…
manifests_in_src = set(re.findall(r'(?:Name:)\s*(.*)',
manifests_in_src))
manifests_in_dest = set(re.findall(r'(?:Name:)\s*(.*)',
manifests_in_dest))

self.assertEqual(tags_in_src, tags_in_dest)
self.assertEqual(manifests_in_src, manifests_in_dest)

def test_task_succeeded(self):
"""Assert that "Copied" appears in stdout."""
self.assertIn('Copied', self.completed_proc.stdout)
123 changes: 123 additions & 0 deletions pulp_smash/tests/docker/cli/utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
# coding=utf-8
"""Common utilities that assist in testing the CLI with docker repositories."""
from __future__ import unicode_literals

import unittest2

from pulp_smash import cli, config, constants, utils


def copy(server_config, unit_type, src_repo_id, dest_repo_id):
"""Use pulp-admin to copy content from one docker repository to another.

:param pulp_smash.config.ServerConfig server_config: Information about the
Pulp server targeted by this function.
:param unit_type: The type of content to copy, such as "image" or
"manifest." Run ``pulp-admin docker repo copy --help`` to get the full
set of available unit types.
:param src_repo_id: A value for the ``--from-repo-id`` option.
:param src_repo_id: A value for the ``--to-repo-id`` option.
"""
cmd = 'pulp-admin docker repo copy {} --from-repo-id {} --to-repo-id {}'
cmd = cmd.format(unit_type, src_repo_id, dest_repo_id).split()
return cli.Client(server_config, cli.echo_handler).run(cmd)


def create_repo(
server_config,
repo_id,
upstream_name=None,
sync_v1=False,
sync_v2=False):
"""Use pulp-admin to create a repo with the given parameters.

:param pulp_smash.config.ServerConfig server_config: Information about the
Pulp server targeted by this function.
:param repo_id: A value for the ``--repo-id`` option.
:param upstream_name: A value for the ``--upstream-name`` option.
:param sync_v1: A value for the ``--enable-v1`` option.
:param sync_v2: A value for the ``--enable-v2`` option.
"""
extra_flags = ''
if upstream_name:
extra_flags += ' --upstream-name {}'.format(upstream_name)

# Handle whether we are syncing, and if so which APIs
if sync_v1 or sync_v2:
# The Docker v2 feed URL can do Docker v1 as well
extra_flags += ' --feed {}'.format(constants.DOCKER_V2_FEED_URL)
if sync_v1:
extra_flags += ' --enable-v1 true'
else:
extra_flags += ' --enable-v1 false'
if sync_v2:
extra_flags += ' --enable-v2 true'
else:
extra_flags += ' --enable-v2 false'

command = 'pulp-admin docker repo create --repo-id {}{}'
command = command.format(repo_id, extra_flags).split()
return cli.Client(server_config, cli.echo_handler).run(command)


def delete_repo(server_config, repo_id):
"""Delete the repo given by repo_id."""
cmd = 'pulp-admin docker repo delete --repo-id {}'.format(repo_id).split()
return cli.Client(server_config, cli.echo_handler).run(cmd)


def search(server_config, unit_type, repo_id, fields=None):
"""
Search the given repo for all units of given unit_type.

unit_type should be a string: "image", "blob", "manifest", or "tag".
repo_id is the repo_id you wish to search.
fields is a list of strings of field names you want to retrieve.
"""
extra_flags = ''
if fields:
extra_flags += ' --fields {}'.format(','.join(fields))
cmd = 'pulp-admin docker repo search {} --repo-id {}{}'
cmd = cmd.format(unit_type, repo_id, extra_flags).split()
return cli.Client(server_config, cli.echo_handler).run(cmd)


def sync_repo(server_config, repo_id):
"""Synchronize the given repo."""
return cli.Client(server_config, cli.echo_handler).run(
'pulp-admin docker repo sync run --repo-id {}'.format(repo_id).split()
)


class BaseTestCase(unittest2.TestCase):
"""A base class for testing Docker content. It logs in for you."""

@classmethod
def setUpClass(cls):
"""Provide a server config and a repository ID."""
cls.cfg = config.get_config()
cls.repo_id = utils.uuid4()
cmd = 'pulp-admin login -u {} -p {}'.format(*cls.cfg.auth).split()
cli.Client(cls.cfg).run(cmd)

@classmethod
def tearDownClass(cls):
"""Delete the created repository."""
cmd = 'pulp-admin docker repo delete --repo-id {}'.format(cls.repo_id)
cli.Client(cls.cfg).run(cmd.split())


class SuccessMixin(object):
"""Add some common assertion to test cases."""

def test_return_code(self):
"""Assert the "sync" command has a return code of 0."""
self.assertEqual(self.completed_proc.returncode, 0)

def test_task_succeeded(self):
"""Assert the phrase "Task Succeeded" is in stdout."""
self.assertIn('Task Succeeded', self.completed_proc.stdout)

def test_task_failed(self):
"""Assert the phrase "Task Failed" is not in stdout."""
self.assertNotIn('Task Failed', self.completed_proc.stdout)