-addressed notes from PR #807

src/rez/cli/test.py

@@ -46,21 +46,29 @@ def command(opts, parser, extra_arg_groups=None):
     import os.path
     import sys
 
+    # note that argparse doesn't support mutually exclusive arg groups
+    if opts.inplace and (opts.extra_packages or opts.paths or opts.no_local):
+        parser.error(
+            "Cannot use --inplace in combination with "
+            "--extra-packages/--paths/--no-local"
+        )
+
     if opts.paths is None:
         pkg_paths = (config.nonlocal_packages_path
                      if opts.no_local else None)
     else:
         pkg_paths = opts.paths.split(os.pathsep)
         pkg_paths = [os.path.expanduser(x) for x in pkg_paths if x]
 
+    # run test(s)
     runner = PackageTestRunner(
         package_request=opts.PKG,
         package_paths=pkg_paths,
         extra_package_requests=opts.extra_packages,
         dry_run=opts.dry_run,
         stop_on_fail=opts.stop_on_fail,
         use_current_env=opts.inplace,
-        verbose=(3 if opts.verbose else 2)
+        verbose=2
     )
 
     test_names = runner.get_test_names()
@@ -71,6 +79,9 @@ def command(opts, parser, extra_arg_groups=None):
         sys.exit(0)
 
     if opts.list:
+        if sys.stdout.isatty():
+            print("Tests defined in %s:" % uri)
+
         print('\n'.join(test_names))
         sys.exit(0)
 
@@ -88,10 +99,16 @@ def command(opts, parser, extra_arg_groups=None):
             )
             sys.exit(0)
 
+    exitcode = 0
+
     for test_name in run_test_names:
         if not runner.stopped_on_fail:
-            runner.run_test(test_name)
+            ret = runner.run_test(test_name)
+            if ret and not exitcode:
+                exitcode = ret
 
     print("\n")
     runner.print_summary()
     print('')
+
+    sys.exit(exitcode)

src/rez/package_maker__.py

@@ -36,7 +36,8 @@
             Optional("on_variants"): Or(
                 bool,
                 {
-                    "requires": [package_request_schema]
+                    "type": "requires",
+                    "value": [package_request_schema]
                 }
             )
         })

src/rez/package_resources_.py

@@ -97,7 +97,8 @@ def late_bound(schema):
             Optional("on_variants"): Or(
                 bool,
                 {
-                    "requires": [
+                    "type": "requires",
+                    "value": [
                         Or(PackageRequest, And(basestring, Use(PackageRequest)))
                     ]
                 }

src/rez/package_serialise.py

@@ -62,7 +62,8 @@
             Optional("on_variants"): Or(
                 bool,
                 {
-                    "requires": [package_request_schema]
+                    "type": "requires",
+                    "value": [package_request_schema]
                 }
             )
         })

src/rez/package_test.py

@@ -1,9 +1,9 @@
 from rez.config import config
 from rez.resolved_context import ResolvedContext
 from rez.packages_ import get_latest_package_from_string, Variant
-from rez.exceptions import PackageNotFoundError, PackageTestError
+from rez.exceptions import RezError, PackageNotFoundError, PackageTestError
 from rez.utils.colorize import heading, Printer
-from rez.utils.logging_ import print_warning
+from rez.utils.logging_ import print_info, print_warning, print_error
 from rez.vendor.six import six
 from rez.vendor.version.requirement import Requirement, RequirementList
 from pipes import quote
@@ -43,16 +43,6 @@ class PackageTestRunner(object):
 
     Commands can also be a list - in this case, the test process is launched
     directly, rather than interpreted via a shell.
-
-    TODO FIXME: Currently a test will not be run over the variants of a
-    package. This is because there is no reliable way to resolve to a specific
-    variant's env - we can influence the variant chosen, knowing how the
-    variant selection mode works, but this is not a guarantee and it would
-    be error-prone and complicated to do it this way. For reasons beyond
-    package testing, we want to be able to explicitly specify a variant to
-    resolve to anyway, so this will be fixed in a separate feature. Once that
-    is available, this code will be updated to iterate over a package's
-    variants and run tests in each.
     """
     def __init__(self, package_request, use_current_env=False,
                  extra_package_requests=None, package_paths=None, stdout=None,
@@ -91,7 +81,7 @@ def __init__(self, package_request, use_current_env=False,
 
         if isinstance(verbose, bool):
             # backwards compat, verbose used to be bool
-            self.verbose = 3 if verbose else 0
+            self.verbose = 2 if verbose else 0
         else:
             self.verbose = verbose
 
@@ -246,8 +236,14 @@ def run_test(self, test_name):
 
         Args:
             test_name (str): Name of test to run.
+
+        Returns:
+            int: Exit code of first failed test, or 0 if none failed. If the first
+                test to fail did so because it was not able to run (eg its
+                environment could not be configured), -1 is returned.
         """
         package = self.get_package()
+        exitcode = 0
 
         if test_name not in self.get_test_names():
             raise PackageTestError("Test '%s' not found in package %s"
@@ -292,44 +288,27 @@ def run_test(self, test_name):
             requires = test_info["requires"]
             on_variants = test_info["on_variants"]
 
-            # if on_variants is a dict containing "requires", then only run the
-            # test on variants whose direct requirements are a subset of, and do
-            # not conflict with, this requires list. Variants are silently skipped
-            # on mismatch in this case. For example, if on_variants.requires is
-            # ['foo', 'bah'] then only variants containing both these requirements
-            # will be selected; ['!foo', 'bah'] would select those variants with
-            # bah present and not foo.
-            #
-            # This is different to the test's "requires" - in this case,
-            # requires=['foo'] would add this requirement to every test env.
-            # This is a requirement addition, whereas on_variants.requires is a
-            # variant selection mechanism.
-            #
-            if isinstance(on_variants, dict) and "requires" in on_variants:
-                reqlist = RequirementList(
-                    variant.variant_requires + on_variants["requires"])
-
-                if reqlist.conflict:
-                    if self.verbose > 2:
-                        self._add_test_result(
-                            test_name,
-                            variant,
-                            "skipped",
-                            "Test skipped as specified by on_variants.requires"
-                        )
-                    continue
+            # apply variant selection filter if specified
+            if isinstance(on_variants, dict):
+                filter_type = on_variants["type"]
+                func = getattr(self, "_on_variant_" + filter_type)
+                do_test = func(variant, on_variants)
+
+                if not do_test:
+                    reason = (
+                        "Test skipped as specified by on_variants '%s' filter"
+                        % filter_type
+                    )
+
+                    print_info(reason)
+
+                    self._add_test_result(
+                        test_name,
+                        variant,
+                        "skipped",
+                        reason
+                    )
 
-                # test if variant requires is a subset of on_variants.requires.
-                # This works because RequirementList merges requirements.
-                #
-                if RequirementList(variant.variant_requires) != reqlist:
-                    if self.verbose > 2:
-                        self._add_test_result(
-                            test_name,
-                            variant,
-                            "skipped",
-                            "Test skipped as specified by on_variants.requires"
-                        )
                     continue
 
             # show progress
@@ -353,26 +332,37 @@ def run_test(self, test_name):
             requires.extend(map(str, variant.variant_requires))
 
             # create test runtime env
-            context = self._get_context(requires)
-
-            if context is None:
+            exc = None
+            try:
+                context = self._get_context(requires)
+            except RezError as e:
+                exc = e
+
+            fail_reason = None
+            if exc is not None:
+                fail_reason = "The test environment failed to resolve: %s" % exc
+            elif context is None:
+                fail_reason = "The current environment does not meet test requirements"
+            elif not context.success:
+                fail_reason = "The test environment failed to resolve"
+
+            if fail_reason:
                 self._add_test_result(
                     test_name,
                     variant,
-                    "skipped",
-                    "The current environment does not meet test requirements"
+                    "failed",
+                    fail_reason
                 )
 
-                print("(Skipped)")
-                continue
+                print_error(fail_reason)
+
+                if not exitcode:
+                    exitcode = -1
+
+                if self.stop_on_fail:
+                    self.stopped_on_fail = True
+                    return exitcode
 
-            if not context.success:
-                self._add_test_result(
-                    test_name,
-                    variant,
-                    "skipped",
-                    "The test environment failed to resolve"
-                )
                 continue
 
             # check that this has actually resolved the variant we want
@@ -437,9 +427,12 @@ def run_test(self, test_name):
                     "Test failed with exit code %d" % retcode
                 )
 
+                if not exitcode:
+                    exitcode = retcode
+
                 if self.stop_on_fail:
                     self.stopped_on_fail = True
-                    return
+                    return exitcode
 
                 continue
 
@@ -455,6 +448,8 @@ def run_test(self, test_name):
             if on_variants is False:
                 break
 
+        return exitcode
+
     def print_summary(self):
         self.test_results.print_summary()
 
@@ -469,6 +464,30 @@ def _print_header(cls, txt, *nargs):
         pr = Printer(sys.stdout)
         pr(txt % nargs, heading)
 
+    def _on_variant_requires(self, variant, params):
+        """
+        Only run test on variants whose direct requirements are a subset of, and
+        do not conflict with, the list given in 'value' param.
+
+        For example, if on_variants.requires is ['foo', 'bah'] then only variants
+        containing both these requirements will be selected; ['!foo', 'bah'] would
+        select those variants with bah present and not foo.
+        """
+        requires_filter = params["value"]
+
+        reqlist = RequirementList(variant.variant_requires + requires_filter)
+
+        if reqlist.conflict:
+            return False
+
+        # test if variant requires is a subset of given requires filter.
+        # This works because RequirementList merges requirements.
+        #
+        if RequirementList(variant.variant_requires) != reqlist:
+            return False
+
+        return True
+
     def _get_test_info(self, test_name, variant):
         tests_dict = variant.tests or {}
         test_entry = tests_dict.get(test_name)
@@ -585,7 +604,11 @@ def _get_target_variants(self, test_name):
                 # otherwise the initial stream of (potentially failing) context
                 # output would be confusing to the user.
                 #
-                context = self._get_context(requires, quiet=True)
+                try:
+                    context = self._get_context(requires, quiet=True)
+                except RezError:
+                    continue
+
                 if context is None or not context.success:
                     continue
 

src/rezplugins/build_process/local.py

@@ -482,7 +482,11 @@ def _run_tests(self, variant, run_on, package_install_path):
             runner.print_summary()
 
         if runner.num_failed:
-            raise PackageTestError("%d tests failed" % runner.num_failed)
+            print('')
+            raise PackageTestError(
+                "%d tests failed; the installation has been cancelled"
+                % runner.num_failed
+            )
 
 
 def register_plugin():

wiki/pages/Package-Definition-Guide.md

@@ -669,7 +669,8 @@ give you the latest possible version.
         "maya_CI": {
             "command": "python {root}/ci_tests/maya.py",
             "on_variants": {
-                "requires": ["maya"]
+                "type": "requires",
+                "value": ["maya"]
             },
             "run_on": "explicit"
         }
@@ -696,10 +697,9 @@ If you provide a nested dict, you can specify extra fields per test, as follows:
   * False (the default): Run the test only on one variant (ie the variant you get by
     default when the test env is resolved). This is useful for tests like linting,
     where variants may be irrelevant.
-  * A dict containing a "requires" field. This is a variant selection mechanism. In the example
-    above, the "maya_CI" test will run only on those variants that directly require `maya` (or a
-    package within this range, eg `maya-2019`). (TODO add a "testing" page to the wiki, and link
-    to it from here).
+  * A dict. This is a variant selection mechanism. In the example above, the "maya_CI" test will
+    run only on those variants that directly require `maya` (or a package within this range, eg
+    `maya-2019`). Note that "requires" is the only filter type currently available.
 
 ### tools
 *List of string*