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

Rep001 1 (rez-test improvements) #807

Merged
merged 7 commits into from
Jan 18, 2020
Merged
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
50 changes: 38 additions & 12 deletions src/rez/cli/test.py
Original file line number Diff line number Diff line change
@@ -8,15 +8,26 @@ def setup_parser(parser, completions=False):
parser.add_argument(
"-l", "--list", action="store_true",
help="list package's tests and exit")
parser.add_argument(
"--dry-run", action="store_true",
help="dry-run mode: show what tests would have been run, but do not "
"run them")
parser.add_argument(
"-s", "--stop-on-fail", action="store_true",
help="stop on first test failure")
parser.add_argument(
"--inplace", action="store_true",

Choose a reason for hiding this comment

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

Should --extra-packages and --inplace be mutually exclusive? In theory it doesn't make much sense to let a user use --extra-package if it's gonna be ignored... Or would it be ignored right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, but agreed, they should be mutually exclusive to avoid confusion.

help="run tests in the current environment. Any test whose requirements "
"are not met by the current environment is skipped")
PKG_action = parser.add_argument(
"--extra-packages", nargs='+', metavar="PKG",
help="extra packages to add to test environment")
parser.add_argument(
"--paths", type=str, default=None,
help="set package search path")
parser.add_argument(
"--nl", "--no-local", dest="no_local", action="store_true",
help="don't load local packages")
PKG_action = parser.add_argument(
"--extra-packages", nargs='+', metavar="PKG",
help="extra packages to add to test environment")
PKG_action = parser.add_argument(
"PKG",
help="package run tests on")
@@ -42,14 +53,20 @@ 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,
package_paths=pkg_paths,
extra_package_requests=opts.extra_packages,
verbose=True)
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=True
)

test_names = runner.get_test_names()
uri = runner.get_package().uri

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

@@ -60,10 +77,19 @@ def command(opts, parser, extra_arg_groups=None):
if opts.TEST:
run_test_names = opts.TEST
else:
run_test_names = test_names
# if no tests are explicitly specified, then run only those with a
# 'default' run_on tag
run_test_names = runner.get_test_names(run_on=["default"])

if not run_test_names:
print(
"No tests with 'default' run_on tag found in %s" % uri,
file=sys.stderr
)
sys.exit(0)

for test_name in run_test_names:
returncode = runner.run_test(test_name)
if not runner.stopped_on_fail:
runner.run_test(test_name)

if returncode:
sys.exit(returncode)

Choose a reason for hiding this comment

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

We no more exit with non-zero exit codes? Is that intentional or on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's intentional, as it isn't clear what this would mean anymore.

Previously, rez-test raised as soon as a test fails. It doesn't do that anymore (unless you specify --stop-on-fail), and it also iterates over variants. So now, you get a summary of all the tests, showing whether they passed/failed/skipped. In that context it isn't really clear what the exit code should be, or if it's useful to have at all.

Choose a reason for hiding this comment

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

I have the feeling it becomes unclear because the tests are skipped instead of being marked as failed. I've been thinking about it even more today and I really think tests that fail to resolve should really be marked as failed. That would avoid hiding possible issues and would probably make it possible to use a different exit code more easily.

A lot of CI system base themselves on the exit code of processes to know if a step failed or not. If we don't return a proper exit code, it's going to be quite difficult to know when a test failed or not and mark a build as failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I'll mark those as failed, and set exit code to match exit code of first failed test.

runner.print_summary()
9 changes: 8 additions & 1 deletion src/rez/package_maker__.py
Original file line number Diff line number Diff line change
@@ -31,7 +31,14 @@
Or(basestring, [basestring]),
{
"command": Or(basestring, [basestring]),
Optional("requires"): [package_request_schema]
Optional("requires"): [package_request_schema],
Optional("run_on"): Or(basestring, [basestring]),
Optional("on_variants"): Or(
bool,
{
"requires": [package_request_schema]

Choose a reason for hiding this comment

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

I'm curious here, how would you use this? If I understand correctly, this is a selection mechanism. So you can select the variants that the test should run on. So you can only have one filter?

Also, why not call the attribute selection or filter? requiressounds too much like the packagerequiresand the testsrequires. Or maybe matches`?

Copy link
Contributor Author

@nerdvegas nerdvegas Dec 3, 2019

Choose a reason for hiding this comment

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

"on_variants" is the scope that you would consider as the filter, whereas the fields within ("requires") can be thought of as the type of filter. So this reads like "run the test on variants that require X". For eg, I haven't added it here (and probably won't) but in theory it would make sense to also have an "index" filter option, within the on_variants dict. And "on_variants" can't be named "filter" because that's too generic. We could go for "variants_filter", but is that any better than "on_variants"?

Example of use: on_variants.requires=["maya"] will run only on variants that (directly) require maya; on_variants.requires=["!nuke"] would run on all non-nuke variants.

I suppose you could have multiple 'requires' filters that would OR together, but I'm not sure that would ever be useful, and it would get quite confusing with conflict ops present. Ie, what would "!maya OR !nuke: mean? That would be the same as no requires filter at all. ANDed options aren't necessary, because you only need a single 'requires' field to describe that (eg ["maya", "!python-3"] will select any maya variant not using python-3).

Choose a reason for hiding this comment

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

Note that my comment was not on the on_variant but on the requires key (from your comment I have the feeling you thought I was talking about the on_variant name...)

What about:

"on_variant": {
    "filter_type" (Or just type): "requirements", (eventually could be index too)
    "filter": ["maya"]
}

I feel this would make the intention clearer here than having yet another property called requires.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is ok, and if different filter types are added, this will avoid some confusion.

I would go with type/value though rather than filter_type/filter (the on_variant dict is the filter)

Choose a reason for hiding this comment

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

Great. I'm fine with type and value.

}
)
}
)
})
11 changes: 10 additions & 1 deletion src/rez/package_resources_.py
Original file line number Diff line number Diff line change
@@ -92,7 +92,16 @@ def late_bound(schema):
"command": Or(basestring, [basestring]),
Optional("requires"): [
Or(PackageRequest, And(basestring, Use(PackageRequest)))
]
],
Optional("run_on"): Or(basestring, [basestring]),
Optional("on_variants"): Or(
bool,
{
"requires": [
Or(PackageRequest, And(basestring, Use(PackageRequest)))
]
}
)
}
)
})
9 changes: 8 additions & 1 deletion src/rez/package_serialise.py
Original file line number Diff line number Diff line change
@@ -56,7 +56,14 @@
Or(basestring, [basestring]),
{
"command": Or(basestring, [basestring]),
Optional("requires"): [package_request_schema]
Optional("requires"): [package_request_schema],
Optional("run_on"): Or(basestring, [basestring]),
Optional("on_variants"): Or(
bool,
{
"requires": [package_request_schema]
}
)
}
)
})
Loading