Fix up Pulp Issue #1937

[danuzclaudes]

This commit fixes the issue: #269.

It creates two repositories with the same feed content. By synchronizing both repos, check if the 2nd repo would have content; otherwise, the sync will fail.

Test output:

[vagrant@dev pulp-smash]$ python -m unittest pulp_smash.tests.puppet.cli
.test_sync_from_puppet_skip_duplicate_content.SyncFromPuppetSkipDuplicateContentTestCase
F
======================================================================
FAIL: test_sync_puppet_forge (pulp_smash.tests.puppet.cli
.test_sync_from_puppet_skip_duplicate_content.SyncFromPuppetSkipDuplicateContentTestCase)
Create two Puppet repositories and trigger their syncs.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "pulp_smash/tests/puppet/cli/test_sync_from_puppet_skip_duplicate_content.py", line 66, in test_sync_puppet_forge
    self.assertIsNot(first_repo_counts, 0, 'Counts should be non-zero.')
AssertionError: unexpectedly identical: 0 : Counts should be non-zero.

----------------------------------------------------------------------
Ran 1 test in 13.514s

FAILED (failures=1)

The test fails due to either of the two cases:

1. At a clean state of the database, the 1st repository would have non-zero content counts, while the 2nd has no content.
2. Otherwise, neither the 1st nor the 2nd repository would have content.

[1] https://pulp.plan.io/issues/1937

[Ichimonji10]

The chance of producing duplicate values is approximately 1 in 340,000,000,000,000,000,000,000,000,000,000,000,000. No need to check for duplicates.

What happens if this test needs a third repo? Should a variable named repo_id3 be created? Naaah. Use a list to store this information.

[danuzclaudes]

@Ichimonji10: so a simple repos = list(); repos.append(utils.uuid4()); repos.append(utils.uuid4()) would be fine?

[Ichimonji10]

Yes, that's fine. A more concise way to write that is to use a list comprehension:

repo_ids = [utils.uuid4() for _ in range(2)]

[Ichimonji10]

Please add some assertions verifying Pulp's behaviour. Just because a sync succeeded doesn't mean that the desired content units are actually present in a repository. A good check would be to run pulp-admin puppet repo list (or some similar command) against each repository, and ensuring that both repositories have an identical non-zero number of content units present.

Generate documentation with make docs-html and run git diff. You should see some additional changes in docs/ that should be added.

[coveralls]

Coverage remained the same at 70.196% when pulling 3c39e0e on danuzclaudes:2.9-issues-automation into a91b1ee on PulpQE:master.

[Ichimonji10]

Heh. I appreciate thorough documentation. Even so, this is a bit verbose. Aim to tell the reader what a test does, but not how the test does that. We want to be able to do things like change the --queries=torssh in our test without having to also update this docstring.

[danuzclaudes]

@Ichimonji10 Thank you for the comments!

While trying to extract the content-unit-counts for each repository, I found out a very subtle detail of this issue. The very first repository, at a clean state of db, would have count of 1 as:

Id:                   repo1
Content Unit Counts:  
  Puppet Module: 1

However, for the rest of repositories, they would only have counts as:

Id:                   repo2
Content Unit Counts:

And even though all those repositories have been deleted, the following new repos would still have no counts, until an execution of pclean.

In that case. the code has been revised to react to the above output; it will verify that counts are non-zero and identical.

[coveralls]

Coverage remained the same at 70.196% when pulling 34f21f1 on danuzclaudes:2.9-issues-automation into 824f1c1 on PulpQE:master.

[Ichimonji10]

While trying to extract the content-unit-counts for each repository, I found out [...]

👍 I found the same thing.

[coveralls]

Coverage remained the same at 70.196% when pulling 08d63df on danuzclaudes:2.9-issues-automation into 824f1c1 on PulpQE:master.

[Ichimonji10]

What happens if there's more than one type of content unit in this repository? For example:

>>> mytext = (
...   'foo bar biz baz\n'
...   'Puppet Module: 5\n'
...   'one two three four'
... )
>>> mytext.split('Puppet Module:')
['foo bar biz baz\n', ' 5\none two three four']

In this case, the call to int(output[1].strip()) will fail. A more robust solution is to find the one line of text containing the keyword 'Puppet Module:', and to parse that one line. Also, this logic seems like something that other test cases might want to make use of. A standalone function will be more re-usable.

Use a function like the following instead:

def _get_n_puppet_modules(server_config, repo_id):
    """Tell how many puppet modules are in a repository.

    :param pulp_smash.config.ServerConfig server_config: Information about the
        Pulp server being targeted.
    :param repo_id: A Puppet repository ID.
    :returns: The number of puppet modules in a repository, as an ``int``.
    """
    keyword = 'Puppet Module:'
    completed_proc = cli.Client(server_config).run((
        'pulp-admin puppet repo list --repo-id {} --fields content_unit_counts'
    ).format(repo_id).split())
    lines = [
        line for line in completed_proc.stdout.splitlines() if keyword in line
    ]
    # If puppet modules are present, a "Puppet Module: n" line is printed.
    # Otherwise, nothing is printed.
    assert len(lines) in (0, 1)
    if len(lines) == 0:
        return 0
    else:
        return int(lines[0].split(keyword)[1].strip())

[coveralls]

Coverage remained the same at 70.196% when pulling 4ed1881 on danuzclaudes:2.9-issues-automation into 824f1c1 on PulpQE:master.

[coveralls]

Coverage remained the same at 70.196% when pulling ca6a35e on danuzclaudes:2.9-issues-automation into 824f1c1 on PulpQE:master.

[Ichimonji10]

Merged, with a follow-up commit of 56f4778. Closed #269. Thank you, @danuzclaudes!

[elyezer]

🎉