Skip tests affected by bug 695

[Ichimonji10]

Add a decorator for skipping tests, pulp_smash.utils.require. Use it to skip a test that is known to fail under Pulp 2.6 and older. See individual commit messages for more details.

[Ichimonji10]

At this point, you have to manually configure Pulp Smash and tell it which version of Pulp you're targeting, and there's also no integration with the bug tracker at pulp.plan.io. But it's a start, and it demonstrates how to use the new "version" attribute on the pulp_smash.config.ServerConfig object.

[bmbouter]

This looks good. I do have one suggestion. Can a decorator be introduced? It would be like this:

     @requires('2.7')
     # 'https://pulp.plan.io/issues/695'
     def test_location_header(self):

Or maybe this:

     @requires('2.7', 'https://pulp.plan.io/issues/695')
     def test_location_header(self):

[Ichimonji10]

Yes, we can do that. The problem with using decorators here is that they are evaluated when defined, but we don't want to be instantiating ServerConfig objects and inspecting their "version" attribute or possibly calling @skip until the tests are executed. As a consequence, the decorators are going to be complicated and bug-prone.

It's definitely doable. It's done in Robottelo, and @elyezer in particular has expertise with writing them. But I'm nervous about adopting something like that right away. This approach is not as concise, but it serves the lowest common denominator. My spidey senses tell me to do other things (like integrate with pulp.plan.io and expand the test suite) first.

[Ichimonji10]

By the way, I do like the two-argument form you've given. I like the idea of requiring that users provide a bug URL in order to skip a test. It might also allow for some nice analytics down the line when we get good TCMS integration in place.

[bmbouter]

The complexity of introducing a decorator versus integrating with a web server are significantly different. Regarding making a decorator I could write one up and post it here if that is helpful.

I really think this is an idiom we want to include from the start. I'm not concerned if ServerConfigs get instantiated a whole lot. We could easily add a caching there so it's only read from disk the first time.

What do you think?

Also if we want to be sure it's a bug (which could be good to require) we could use it like:

@requires('2.7', 695)

[Ichimonji10]

We could easily add a caching there so it's only read from disk the first time.

pulp_smash.config.get_config() already has caching, so instantiating lots of objects isn't an issue.

[Ichimonji10]

What I'm much more worried about is that ServerConfig objects might be instantiated when each decorator is evaluated, and that they would persist until when each test is run. I'd strongly prefer if ServerConfig objects were instantiated as lazily as possible. There's no problem with the agressive instantiation approach at this time, but it invites unexpected behaviour should we ever need to do something wild like clear or edit the pulp_smash.config._CONFIG cache at run-time.

[Ichimonji10]

But hey, if we can get a well-engineered decorator now - that'd be great. Please do put something together if you have a few minutes. 😄

[Ichimonji10]

@bmbouter Actually, is there even any need to state the version at which a bug is fixed? We could just do something like this:

@skip_if_open(695)
def test_foo(self):
    pass

And at run-time, we can grab a ServerConfig object and fetch bug info from the server, and compare the version on the former with the "target release" attribute from the latter.

[dkliban]

This would require integration with Redmine at this time. It would be advantageous to hold off on doing that integration till later.

[Ichimonji10]

Why so? Integration is extremely easy.

[dkliban]

Even though it is easy, it is still another step that we have to take before we can start skipping tests.

[Ichimonji10]

We can skip tests right now!

[Ichimonji10]

Basically, if we're going to go above and beyond serving the lowest common denominator, let's make sure what we come up with is orthogonal to what we already have.

[bmbouter]

Here is a simple decorator for it. Personally I think introducing the decorator is good to do now. More than redmine integration, I think all the core tools bugs need to be done. I like the integration, but I don't think it's blocking test writing. I'm still blocked on #31

def require(func, version_string):
    def new_func(self, *args, **kwargs):
        if self.cfg.version < Version(version_string):
            self.skipTest('Test requires version %s' % version_string)
        else:
            return func(self, *args, **kwargs)
    return new_func

[Ichimonji10]

@bmbouter Concise. I like it.

[Ichimonji10]

I'll add that as a second commit in this PR and ask for a re-review then.

This may come in handy later:

#!/usr/bin/env python
"""Fetch the target release of a bug.

This can fail in at least two obvious cases:

* There is an error while communicating with the server. (e.g. cannot be
  reached)
* There is no "target release" set on the bug.

In either case, an appropriately cautious response is to assume that the bug is
applicable and open.

"""
from __future__ import print_function

import requests


response = requests.get('https://pulp.plan.io/issues/695.json')
response.raise_for_status()
target_releases = [
    custom_field for custom_field in response.json()['issue']['custom_fields']
    if custom_field['id'] == 4  # 'name': 'Target Release'
]
assert len(target_releases) == 1
print(target_releases[0]['value'])

[Ichimonji10]

Decorator usage boils down to this:

@require('2.7')  # https://pulp.plan.io/issues/695
def test_foo(self):
    pass

I implemented the decorator so as to fail if an invalid version string is passed. This will throw an exception:

@require('2.7.foobar')  # https://pulp.plan.io/issues/695
def test_foo(self):
    pass

[bmbouter]

@Ichimonji10 nice! I really like it.

[Ichimonji10]

I've manually verified this change one more time. When the test suite is run
against Pulp 2.6, we've correctly removed two failures and added one skipped
test.

branch         results
-------------  ---------------------------------------
master         FAILED (failures=9, errors=1, skipped=5)
skip-failures  FAILED (failures=7, errors=1, skipped=6)

When the test suite is run against Pulp 2.7, nothing changes:

branch         results
-------------  ---------------------------------------
master         FAILED (failures=8, errors=1, skipped=5)
skip-failures  FAILED (failures=8, errors=1, skipped=5)

Merging changes now.