Add tests cases for copying Docker units. #126
Conversation
| from pulp_smash import cli, config, utils | ||
|
|
||
|
|
||
| FEED = 'https://index.docker.io' |
There was a problem hiding this comment.
As of d93de79, there are now docker feed constants. Can we use one of those constants, or at least make FEED private?
By the way, as per our discussion on the mailing lists, I'm not entirely sure if those constants have correct values. It might make more sense to have something like this in the constants file:
DOCKER_V1_FEED_URL = '...'
DOCKER_V2_FEED_URL = DOCKER_V1_FEED_URLOr even something else. Basically: if the docker-related constants don't seem right to you, please speak up and/or submit a pull request.
There was a problem hiding this comment.
Ah I didn't realize there were constants. Those values are no longer correct, and also there is now one endpoint that does v1 and v2. I'll change my PR to update those constants. We might as well keep them separate in case docker changes them again later.
|
Hmm. Let's start with the easy stuff. One, please document function and method arguments, especially when they're public. Second, don't be afraid to use positional string interpolation - the results are often more legible. Third, please avoid hanging indents. It's a purely stylistic thing, but they're used no-where else in the codebase right now. Here's a diff that implements these three suggestions for the diff --git a/pulp_smash/tests/docker/cli/utils.py b/pulp_smash/tests/docker/cli/utils.py
index 1802cf3..84e83d9 100644
--- a/pulp_smash/tests/docker/cli/utils.py
+++ b/pulp_smash/tests/docker/cli/utils.py
@@ -11,26 +11,43 @@ FEED = 'https://index.docker.io'
def copy(server_config, unit_type, src_repo_id, dest_repo_id):
- """Use pulp-admin to copy all instances of unit_type from src to dest."""
- command = ('pulp-admin docker repo copy {unit_type} --from-repo-id {src} '
- '--to-repo-id {dest}')
- command = command.format(unit_type=unit_type, src=src_repo_id,
- dest=dest_repo_id)
- command = command.split()
-
- return cli.Client(server_config, cli.echo_handler).run(command)
-
-
-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."""
+ """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 {n}'.format(n=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:
- extra_flags += ' --feed {f}'.format(f=FEED)
+ extra_flags += ' --feed {}'.format(FEED)
if sync_v1:
extra_flags += ' --enable-v1 true'
else:
@@ -40,19 +57,15 @@ def create_repo(server_config, repo_id, upstream_name=None, sync_v1=False,
else:
extra_flags += ' --enable-v2 false'
- command = ('pulp-admin docker repo create --repo-id '
- '{repo_id}{extra_flags}').format(
- repo_id=repo_id, extra_flags=extra_flags).split()
-
+ 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."""
- command = 'pulp-admin docker repo delete --repo-id {repo_id}'.format(
- repo_id=repo_id).split()
-
- return cli.Client(server_config, cli.echo_handler).run(command)
+ 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):
@@ -65,14 +78,10 @@ def search(server_config, unit_type, repo_id, fields=None):
"""
extra_flags = ''
if fields:
- extra_flags += ' --fields {f}'.format(f=','.join(fields))
-
- command = ('pulp-admin docker repo search {unit_type} --repo-id '
- '{repo_id}{eflags}')
- command = command.format(unit_type=unit_type, repo_id=repo_id,
- eflags=extra_flags).split()
-
- return cli.Client(server_config, cli.echo_handler).run(command)
+ 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):
@@ -83,27 +92,25 @@ def sync_repo(server_config, repo_id):
class BaseTestCase(unittest2.TestCase):
- """A Base class for testing Docker content. It logs in for you."""
+ """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()
- cli.Client(cls.cfg).run(
- 'pulp-admin login -u {} -p {}'
- .format(cls.cfg.auth[0], cls.cfg.auth[1]).split()
- )
+ 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."""
- command = 'pulp-admin docker repo delete --repo-id {}'
- cli.Client(cls.cfg).run(command.format(cls.repo_id).split())
+ 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."""
+ """Add some common assertion to test cases."""
def test_return_code(self):
"""Assert the "sync" command has a return code of 0.""" |
|
I like the idea behind adding a Making re-usable code is hard. Really, I know! In my experience, a good time to introduce re-usable code is when there really are multiple uses for a given piece of code. This has the obvious benefit of ensuring that you've correctly designed the reusable code. But it also has the side benefit of ensuring that changes are better isolated in the commit history, and therefore can be rolled back or understood after-the-fact. And allowing some level of redundancy lets you experiment and find a good solution for the problem at hand. ("Make it work, make it right...") How about adding the new utilities directly into module |
| super(CopyAllImagesTestCase, cls).tearDownClass() | ||
| docker_utils.delete_repo(cls.cfg, cls.copy_target) | ||
|
|
||
| def test_positive_copy_output(self): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Ah, I should mention one more thing: the stubs in the No fun, I know. :( I am under the impression that Sphinx can auto-generate such, but I've not looked into it. |
bowlofeggs
force-pushed
the
98
branch
2 times, most recently
from
26c0d19 to
cf838c5
Compare
|
I don't believe there is value in spending time refactoring this PR to not have reusable code, only to immediately refactor the code again on Monday to have reusable code. That's not a good way to use time just because we might want to undo a commit later (which is also highly unlikely with this completely additive PR.) Let's focus on spending our time on what will benefit us. Furthermore, the sync.py can/will use this same code, and so I wrote it this way with that in mind as per our discussion in IRC yesterday. I did not do that in this PR because @asmacdo was working in the same area. It was upon your advice to integrate them later. |
|
I see. I'll revisit this on Monday. |
This commit adds several test cases for copying Docker units. Additionally, it adds a handy utils module that contains several convenient functions that are useful for these test cases, as well as for other future test cases. Fixes pulp#98
Ahh. So the plan is to take the code in So: if there's immediate re-use planned for the future, that provides some rationale for adding these two modules in one go. I would still rather have atomic commits that make easily reverted, isolated changes. (There are several ways to do this. One option is to place all new test code in a single module and pull out common code in a later commit, as originally suggested. A second option is to pull common code out of the |
|
This past weekend, I spent some time working with the code in this PR. How about I take that work, polish it a bit further, and then post back here when I'm done? It may be easier and less factious to show than to tell. |
|
@Ichimonji10 why not merge this PR as is, and then apply the patch you made on it immediately thereafter? I am eager to move on to other work, and I already have a need to use the utils module from this PR in a follow up PR. |
|
Thanks! This will be useful as I work on https://pulp.plan.io/issues/1710 |
|
Def. I hope the utilities fulfill your needs. Feel free to expand them or add more - I think their structure is very consistent, and I hope you'll find the same. Thank you for providing the tests. |
|
The assertions that the return codes are 0 and the dockblock params in utils are both missing in your commits. You made comments about both of those, so you might want to add those back. |
|
There's no need to assert that CLI return codes are 0. The CLI client does that automatically. See I've documented the meaning of the CLI parameters in the module-wide docstring. See
Individual parameters could still be documented, but they will be redundant: # Yes, this function has an annoying number of arguments. It may be better to
# adopt some other solution such as providing a string for interpolation.
def repo_create( # pylint:disable=too-many-arguments
server_config,
enable_v1=None,
enable_v2=None,
feed=None,
repo_id=None,
upstream_name=None):
"""Execute ``pulp-admin docker repo create``.
:param pulp_smash.config.ServerConfig server_config: Information about the
Pulp server being targeted.
:param enable_v1: A value for ``--enable-v1``.
:param enable_v2: A value for ``--enable-v2``.
:param feed: A value for ``--feed``.
:param repo_id: A value for ``--repo-id``.
:param upstream_name: A value for ``--upstream-name``.
"""It's fine to have this sort of documentation, I suppose. Submit a patch if you'd really like to have them in place. |
|
The CLI raising an Exception when pulp-admin returns a non-0 exit code goes against a principle you were pushing in this pull request. In this pull request, you asserted the importance of having only one assertion per method so that when tests fail we can see the full list of failed assertions (i.e., no previous failure causes a later failure or success to be masked). However, the combination of having the CLI raise an Exception when encountering non-zero exit codes, and having interactions with Pulp in setUpClass() methods means that no assertions will happen when there are bugs in Pulp that generate a non-zero exit code in pulp-admin. I don't personally care either way, but it is frustrating that you made me jump through so many hoops here and then didn't apply that same principle to your code. I wasn't personally interested in the docblocks, but I found it surprising that you placed a demand on their existence but then didn't apply that same demand to your commits, with a message about how you suppose it's fine to have them. Again this is frustrating since you made me jump through these hoops, but you lowered the standard for yourself. |
The functions originally added by this pull request did not have a uniform logic to them. Each method had a unique signature, where the name of each method varied, and where each argument might or might not be required or be given a value. In such a situation, documenting each function individually is necessary. The functions recently added have a uniform logic to them. Each method is named after the pulp-admin command executed. Each argument is named after the corresponding pulp-admin flag, and no |
Test code has the dual responsibilities of providing a rich set of tests while not being cumbersome. Checking for non-zero return codes and ensuring that asynchronous tasks complete successfully are two examples of checks that should be done in almost all tests, but which are also highly redundant. Using
Given these significant benefits, I do use the automatic checks where possible. (See: #122.) In addition, I write somewhat-overlapping sets of tests.
The tests added in 012fa3a have isolated assertions. Check it out - no assertion failure will ever prevent another assertion from executing. Further, nearly every test method already in pulp smash has an isolated set of assertions. |
This commit adds several test cases for copying Docker units.
Additionally, it adds a handy utils module that contains several
convenient functions that are useful for these test cases, as well
as for other future test cases.
Fixes #98