-
Notifications
You must be signed in to change notification settings - Fork 336
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
base: main
Are you sure you want to change the base?
Changes from all commits
55a4d0c
efc247d
bf60dd7
04c56a6
fc8bffd
129c813
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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") | ||
|
@@ -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") | ||
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='?', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
help="package run tests on") | ||
parser.add_argument( | ||
"TEST", nargs='*', | ||
|
@@ -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 | ||
|
@@ -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: | ||
|
@@ -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 == '.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wrong return docstring. Also not sure we need this function. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
""" | ||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
""" | ||
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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) |
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 | ||
|
||
|
@@ -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: | ||
|
@@ -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. | ||
|
@@ -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 | ||
|
@@ -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. | ||
|
@@ -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 | ||
|
@@ -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 '.') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
This is more consistent with how settings are managed in other parts of the codebase. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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) | ||
|
@@ -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) | ||
} |
There was a problem hiding this comment.
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.