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.

Sign up for GitHub

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

Rep001 1 (rez-test improvements) #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6e53473
added rez-test --dry-run mode
Nov 19, 2019
e845368
-add better support for variants
Nov 19, 2019
7cf9c3b
-updated tests schema
Nov 20, 2019
a97581e
improvement to storage of test results
Nov 20, 2019
019a330
-fixed bug in variant iteration logic in test runner
Nov 20, 2019
c55797a
-added 'run_on' tests field
Nov 21, 2019
431d4e5
update wiki docs wrt `tests` package attrib
Nov 21, 2019
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

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")
Expand All @@ -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)

Expand All @@ -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)
Copy link
Member

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.

Copy link
Member

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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

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 •
edited
Loading

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).

Copy link
Member

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)

Copy link
Member

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.

}
)
}
)
})
Expand Down
11 changes: 10 additions & 1 deletion src/rez/package_resources_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)))
]
}
)
}
)
})
Expand Down
9 changes: 8 additions & 1 deletion src/rez/package_serialise.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
)
}
)
})
Expand Down
Loading