integration_tests: introduce skipping of tests by OS

[OddBloke]

Proposed Commit Message

integration_tests: introduce skipping of tests by OS

This introduces an optional, more complex OS_IMAGE format (`<image
id>::<os>::<release>`) which allows the specification of the OS/OS
release which the given image ID corresponds to.  This information is
used to skip tests which do not apply to the image.
 
This commit is comprised of the following discrete changes:
* introduce the IntegrationImage class, to handle parsing and storing
   the new OS_IMAGE format
* support inferring the OS and OS release of Ubuntu series, so that we
  can continue to set OS_IMAGE to just a series name and have test
  skipping work
* add documentation on Image Selection to integration_tests.rst
* introduce the actual skipping behaviour based on OS marks
* apply the `ubuntu` mark to all tests that should be skipped on
  non-Ubuntu operating systems

Additional Context

See the added "Image Selection" doc section for an overview.

Test Steps

Travis should continue to pass, and it should be observed that tests are skipped on non-Ubuntu releases. This can be seen by using an Ubuntu image but lying about its origin with a full OS_IMAGE specification:

$ CLOUD_INIT_OS_IMAGE="ubuntu:focal::notubuntu::noreally" pytest --log-cli-level=INFO tests/integration_tests/ -rsXx
...
INFO     integration_testing:clouds.py:74 Detected image: image_id=ubuntu:focal os=notubuntu release=noreally
...
=== short test summary info ===
SKIPPED [12] tests/integration_tests/conftest.py:69: Cannot run on OS notubuntu

Checklist:

• My code follows the process laid out in the documentation
• I have updated or added any unit tests accordingly
• I have updated or added any documentation accordingly

[TheRealFalcon]

Overall the idea makes sense and seems like a good way forward.

[TheRealFalcon]

Do you foresee this class growing or taking on more functionality? If not, I don't think it needs to be a class. The constructor isn't directly invoked, so that leaves us with the classmethod constructor and a helper for building. Would it make more sense as a function returning a namedtuple?

[OddBloke]

I think we'll be able to write the mark matching logic more succinctly if this class implements a matches(os, release) (or something broadly along those lines). Roughly:

--- a/tests/integration_tests/clouds.py
+++ b/tests/integration_tests/clouds.py
@@ -75,6 +75,11 @@ class ImageSpecification:
             "Image tuple: (%s, %s, %s)", self.image_id, self.os, self.release
         )
 
+    def matches(self, os, release):
+        matches_os = self.os is None or self.os == os
+        matches_release = self.release is None or self.release == release
+        return matches_os and matches_release
+
     @classmethod
     def from_os_image(cls):
         parts = integration_settings.OS_IMAGE.split("::", 2)
diff --git a/tests/integration_tests/conftest.py b/tests/integration_tests/conftest.py
index fc52a8de..b482cfa2 100644
--- a/tests/integration_tests/conftest.py
+++ b/tests/integration_tests/conftest.py
@@ -64,14 +64,7 @@ def pytest_runtest_setup(item):
     image = ImageSpecification.from_os_image()
     if "only_bionic" in test_marks:
         # TODO: implement generic matching logic
-
-        # Run Ubuntu tests if we either know this image is Ubuntu, or don't
-        # know what it is at all
-        run_ubuntu_tests = image.os is None or image.os == "ubuntu"
-        # Run bionic tests if we either know this image is bionic, or don't
-        # know what it is at all
-        run_bionic_tests = image.release is None or image.release == "bionic"
-        if not (run_ubuntu_tests and run_bionic_tests):
+        if not image.matches("ubuntu", "bionic"):
             pytest.skip("Only runs on Ubuntu bionic")

(And even if I didn't think the above: arguably a namedtuple with a function that constructs it is just a class without a namespace to put __init__ in. 😉)

[TheRealFalcon]

Is this supposed to be image_id? I don't see where image_spec comes from.

[OddBloke]

On Tue, Dec 01, 2020 at 02:51:37PM -0800, James Falcon wrote: @TheRealFalcon commented on this pull request. > except (ValueError, IndexError): - pass - return _released_image_id + return image.image_spec Is this supposed to be `image_id`? I don't see where `image_spec` comes from.

Yep, this is what I get for renaming things right before pushing. :p

[blackboxsw]

LGTM!