Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rez test improvements (dev mode, interactive flag) #759

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 106 additions & 6 deletions src/rez/cli/test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,20 @@
Run tests listed in a package's definition file.
'''
from __future__ import print_function
import sys

import os

from rez.cli._main import run
from rez.config import config
from rez.exceptions import PackageMetadataError
from rez.package_serialise import dump_package_data
from rez.packages_ import get_developer_package

def setup_parser(parser, completions=False):
parser.add_argument(
"-c", "--clean", action="store_true",
help="clear the current tests artifacts before rerunning.")
parser.add_argument(
"-l", "--list", action="store_true",
help="list package's tests and exit")
Expand All @@ -14,11 +25,17 @@ def setup_parser(parser, completions=False):
parser.add_argument(
"--nl", "--no-local", dest="no_local", action="store_true",
help="don't load local packages")
parser.add_argument(
"--interactive", dest="interactive", default=None,
help="resolves an interactive environment for running the tests.")
parser.add_argument(
"--variant", dest="variant", default=0, type=int,
help="variant number which should be used to set the interactive env. Works only with --interactive option")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work in the general case also, it makes sense to specify a variant to run tests on.

PKG_action = parser.add_argument(
"--extra-packages", nargs='+', metavar="PKG",
help="extra packages to add to test environment")
PKG_action = parser.add_argument(
"PKG",
"PKG", nargs='?',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-h does not make it clear what it means if PKG is not provided. It also is not clear from the help that PKG can be a path (ie .). I'm tempted to say that we keep it simpler than this - PKG is either provided or not, and if not, then we're in "developer" mode.

help="package run tests on")
parser.add_argument(
"TEST", nargs='*',
Expand All @@ -32,8 +49,11 @@ def setup_parser(parser, completions=False):
def command(opts, parser, extra_arg_groups=None):
from rez.package_test import PackageTestRunner
from rez.config import config
import os.path
import sys

pkg = get_package(os.getcwd())

if pkg and is_dev_run(opts.PKG):
prepare_dev_env_package(pkg)

if opts.paths is None:
pkg_paths = (config.nonlocal_packages_path
Expand All @@ -42,21 +62,29 @@ def command(opts, parser, extra_arg_groups=None):
pkg_paths = opts.paths.split(os.pathsep)
pkg_paths = [os.path.expanduser(x) for x in pkg_paths if x]

runner = PackageTestRunner(package_request=opts.PKG,
pkg_request = "{name}-{version}".format(name=pkg.name, version=pkg.version) if pkg else opts.PKG
runner = PackageTestRunner(package_request=pkg_request,
package_paths=pkg_paths,
extra_package_requests=opts.extra_packages,
verbose=True)
verbose=True, clean=opts.clean)

test_names = runner.get_test_names()
if not test_names:
uri = runner.get_package().uri
uri = runner.package.uri
print("No tests found in %s" % uri, file=sys.stderr)
sys.exit(0)

if opts.list:
print('\n'.join(test_names))
sys.exit(0)

if opts.interactive and opts.interactive in test_names:
runner.run_test_env(opts.variant, opts.interactive)
sys.exit(0)
elif opts.interactive:
print("Invalid interactive name. Possible choices: {choices}".format(choices=','.join(test_names), file=sys.stderr))
sys.exit(1)

if opts.TEST:
run_test_names = opts.TEST
else:
Expand All @@ -67,3 +95,75 @@ def command(opts, parser, extra_arg_groups=None):

if returncode:
sys.exit(returncode)


def is_dev_run(name):
"""
Decides if is a dev_run, based on the package name
Args:
name: name of package

Returns: True or False
"""
return not name or name == '.'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too fragile. As mentioned earlier, I think it'd be better if developer mode is enabled only if PKG is not provided. Right now is odd because PKG can be a package string, or ".", but not any other valid path. There is too much variability in what PKG can be.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree



def get_package(path):
"""
Function validates pkg name and indicates if tests were run in developer mode
Args:
path: path to the package

Returns: Tuple of values (package_name, is_dev_run)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong return docstring. Also not sure we need this function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably not needed, I'm guessing was added to make the code more testable, but I'm pretty sure you can call directly get_developer_package

"""
try:
return get_developer_package(path)
except PackageMetadataError:
print("Not a valid rez package. Make sure you are in package's root directory", file=sys.stderr)
return None


def prepare_dev_env_package(package):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually pretty hesitant about rez-test possibly implicitly installing a package like this. Package installation is always explicit everywhere else. Furthermore this feels arbitrary - the package gets installed if it wasn't there, but what if it's there but put of date due to a sourcecode change? We can't detect that and I don't think we should.

It seems simpler and less error prone to simply give an error if the current developer package isn't present as a local package, and leave it up to the user if and when to rebuild/reinstall the package.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we can not detect that. ATM we are only detecting changes on the packages.py and reinstalling the package when the tests are out of date.
I'll do a poll tomorrow when I'm at work, I'm guessing not many devs are really relying on this feature. So, I guess it can go away

"""
Prepares a dev environment for tests. If a package is not installed in the local packages path
or the tests on the package[py/yaml] are outdated, it installs the package
or updates the package file with the new test targets.
Args:
package: package to be built
"""
try:
local_package_path = os.path.join(config.local_packages_path, package.name, str(package.version))
local_package = get_developer_package(local_package_path)
except PackageMetadataError:
install_package_in_local_packages_path()
else:
update_package_in_local_packages_path(package, local_package)


def install_package_in_local_packages_path():
"""
Installs the package in the local_packages_path (defined in config.local_packages_path)
"""
original_args = sys.argv
sys.argv = [original_args[0]] + ['-i']
try:
run("build")
except SystemExit as exit_code:
if exit_code.code:
raise
sys.argv = original_args


def update_package_in_local_packages_path(package, installed_package):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again this feels arbitrary - the package is being kept up-to-date wrt its tests, but not its actual payload (ie if source was changed). I think we should just leave it up to the developer to reinstall the package.

Having said that, something that I think would be acceptable in developer mode is if the test definitions are always taken from the developer package, rather than from the installed package.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, if they are taken from the developer package, I think people will be happy

"""
Updates tests package definition in installed packages path
Args:
package: package object of currently processed package
installed_package: package object of installed package
"""
if package.tests != installed_package.tests:
data = installed_package.validated_data()
data['tests'] = package.tests

with open(installed_package.filepath, 'w') as f:
dump_package_data(data, f)
3 changes: 2 additions & 1 deletion src/rez/package_maker__.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@
Or(basestring, [basestring]),
{
"command": Or(basestring, [basestring]),
Optional("requires"): [package_request_schema]
Optional("requires"): [package_request_schema],
Optional("run_in_root"): bool
}
)
})
Expand Down
3 changes: 2 additions & 1 deletion src/rez/package_resources_.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ def late_bound(schema):
"command": Or(basestring, [basestring]),
Optional("requires"): [
Or(PackageRequest, And(basestring, Use(PackageRequest)))
]
],
Optional("run_in_root"): bool
}
)
})
Expand Down
3 changes: 2 additions & 1 deletion src/rez/package_serialise.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@
Or(basestring, [basestring]),
{
"command": Or(basestring, [basestring]),
Optional("requires"): [package_request_schema]
Optional("requires"): [package_request_schema],
Optional("run_in_root"): bool
}
)
})
Expand Down
112 changes: 87 additions & 25 deletions src/rez/package_test.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import shutil

from rez.config import config
from rez.resolved_context import ResolvedContext
from rez.packages_ import get_latest_package_from_string, Variant
from rez.packages_ import get_latest_package_from_string, get_package_from_string, Variant
from rez.exceptions import PackageNotFoundError, PackageTestError
from rez.utils.colorize import heading, Printer
from rez.vendor.six import six
from rez.utils.system import change_dir
from pipes import quote
import subprocess
import os
import time
import sys

Expand Down Expand Up @@ -53,7 +57,7 @@ class PackageTestRunner(object):
"""
def __init__(self, package_request, use_current_env=False,
extra_package_requests=None, package_paths=None, stdout=None,
stderr=None, verbose=False, **context_kwargs):
stderr=None, verbose=False, clean=False, **context_kwargs):
"""Create a package tester.

Args:
Expand All @@ -68,6 +72,7 @@ def __init__(self, package_request, use_current_env=False,
stdout (file-like object): Defaults to sys.stdout.
stderr (file-like object): Defaults to sys.stderr.
verbose (bool): Verbose mode.
clean (bool): States whether clean test artifacts before rerunning
context_kwargs: Extra arguments which are passed to the
`ResolvedContext` instances used to run the tests within.
Ignored if `use_current_env` is True.
Expand All @@ -78,12 +83,13 @@ def __init__(self, package_request, use_current_env=False,
self.stdout = stdout or sys.stdout
self.stderr = stderr or sys.stderr
self.verbose = verbose
self.clean = clean
self.context_kwargs = context_kwargs

self.package_paths = (config.packages_path if package_paths is None
else package_paths)

self.package = None
self._package = None
self.contexts = {}

# use a common timestamp across all tests - this ensures that tests
Expand All @@ -93,35 +99,37 @@ def __init__(self, package_request, use_current_env=False,
if use_current_env:
raise NotImplementedError

def get_package(self):
@property
def package(self):
"""Get the target package.

Returns:
`Package`: Package to run tests on.
"""
if self.package is not None:
return self.package
if self._package:
return self._package

if self.use_current_env:
pass
else:
package = get_latest_package_from_string(str(self.package_request),
self.package_paths)
if package is None:
raise PackageNotFoundError("Could not find package to test: %s"
% str(self.package_request))
package = get_package_from_string(str(self.package_request), self.package_paths)

if not package:
package = get_latest_package_from_string(str(self.package_request), self.package_paths)

self.package = package
return self.package
if not package:
raise PackageNotFoundError("Could not find package to test: %s" % str(self.package_request))

self._package = package
return package

def get_test_names(self):
"""Get the names of tests in this package.

Returns:
List of str: Test names.
"""
package = self.get_package()
return sorted((package.tests or {}).keys())
return sorted((self.package.tests or {}).keys())

def run_test(self, test_name):
"""Run a test.
Expand All @@ -140,16 +148,14 @@ def print_header(txt, *nargs):
pr = Printer(sys.stdout)
pr(txt % nargs, heading)

package = self.get_package()

if test_name not in self.get_test_names():
raise PackageTestError("Test '%s' not found in package %s"
% (test_name, package.uri))
% (test_name, self.package.uri))

if self.use_current_env:
return self._run_test_in_current_env(test_name)

for variant in package.iter_variants():
for variant in self.package.iter_variants():

# get test info for this variant. If None, that just means that this
# variant doesn't provide this test. That's ok - 'tests' might be
Expand Down Expand Up @@ -209,11 +215,20 @@ def print_header(txt, *nargs):

print_header("\nRunning test command: %s\n", cmd_str)

retcode, _, _ = context.execute_shell(
command=command,
stdout=self.stdout,
stderr=self.stderr,
block=True)
test_result_dir = os.path.abspath('build/rez_test_result' if not test_info["run_in_root"] else '.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'build/rez_test_result' should be configurable. As mentioned earlier, we should have a tests_artifact_path setting rather than run_in_root. Then, to run tests in root for a given package, you do a package-level config override, like so:

with scope("config") as c:
    c.tests_artifact_repository = '.'

This is more consistent with how settings are managed in other parts of the codebase.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree


if self.clean and os.path.exists(test_result_dir):
shutil.rmtree(test_result_dir)

if not os.path.exists(test_result_dir):
os.makedirs(test_result_dir)

with change_dir(test_result_dir):
retcode, _, _ = context.execute_shell(
command=command,
stdout=self.stdout,
stderr=self.stderr,
block=True)

if retcode:
return retcode
Expand All @@ -224,6 +239,52 @@ def print_header(txt, *nargs):

return 0 # success

def run_test_env(self, variant_index, test_name):
"""
Runs test env which contains all packages required for any test
Args:
variant_index (int): chosen variant index
test_name (string): name of test
"""
variant = self.package.get_variant(variant_index)

if not variant:
variant = self.package

test_info = self._get_test_info(test_name, variant)

requires = test_info['requires']
context_name = tuple(requires)
context = self.contexts.get(context_name, self._get_test_context(requires))

return_code, _, _ = context.execute_shell()

sys.exit(return_code)

def _get_test_context(self, requires):
"""
Resolves context of test environment basing on required packages
Args:
requires (list): list of required packages

Returns:
Resolved context (ResolvedContext object)
"""
if self.verbose:
self._print_header("Resolving test environment: %s\n", ' '.join(map(quote, requires)))

context = ResolvedContext(package_requests=requires,
package_paths=self.package_paths,
buf=self.stdout,
timestamp=self.timestamp,
**self.context_kwargs)

return context

def _print_header(self, txt, *nargs):
pr = Printer(sys.stdout)
pr(txt % nargs, heading)

def _get_test_info(self, test_name, variant):
tests_dict = variant.tests or {}
test_entry = tests_dict.get(test_name)
Expand Down Expand Up @@ -254,5 +315,6 @@ def _get_test_info(self, test_name, variant):

return {
"command": test_entry["command"],
"requires": requires
"requires": requires,
"run_in_root": test_entry.get("run_in_root", False)
}
Loading