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 --interactive flag #1224

Open
ColinKennedy opened this issue Feb 21, 2022 · 2 comments · May be fixed by #1283
Open

rez-test --interactive flag #1224

ColinKennedy opened this issue Feb 21, 2022 · 2 comments · May be fixed by #1283

Comments

@ColinKennedy
Copy link

ColinKennedy commented Feb 21, 2022

I've been using a custom script to get "a Rez environment of a rez-test" for a while but it has flaws. Looking through Rez's list of issues and PRs, it looks like this has been on the menu for a while (#665).

I'd like to implement this work. Full disclosure, I've already written a mostly-working version of this, including unittests (in this forked branch). But before putting in a PR, I wanted to discuss it a bit more. Reading Fede's PR on the topic (#759) was very insightful. However I discovered a some sharp edges related to tests "requires", "on_variants" and the UX of providing an explicit --variant (or not). I think I got a list of expected behavior that is consistent and makes sense though. I'll outline it below.

Given some_package:

name = "some_package"

# ... more package definition here ...

variants = [["python-2", "backports.functools-lru-cache-1"], ["python-3"]]

tests = {
    "foo": {
        "command": "echo foo",
        "requires": ["python-2", "six"],
    },
    "fizz": {
        "command": "echo another",
        "requires": ["python-2", "extra_package-2"],
    },
    "bar": {
        "command": "echo foo",
        "requires": ["python-3"],
    },
    "buzz": {
        "command": "echo buzz",
        "requires": ["python-3", "thing-1"],
    },
    "lastly": {
        "command": "echo lastly",
        "requires": ["python-3"],
        "run_on": "explicit",
    }
}

Here's how I'm thinking that the CLI would behave:

# Gets the combination of "foo & fizz"
rez-test some_package foo fizz --interactive

# Resolves the "bar & buzz" package. "lastly" is ignored because it is "explicit"
rez-test some_package --variant 1 --interactive

# Resolves with "lastly", explicitly
rez-test some_package lastly --interactive

# Fails to resolve because --variant 0 does not match the requires of "lastly"
rez-test some_package lastly --variant 0 --interactive

# Fails to resolve because "foo / fizz" conflict with "bar", which are all default tests
rez-test some_package --interactive

# Fail to resolve because python versions conflict
rez-test some_package foo bar --interactive

# Resolves if all default tests are compatible
rez-test another_package --interactive

To summarize the behavior, I tried to make --interactive work like the actual rez-env behavior.

  • If --variant is included, the resolve must use that variant or fail
  • If a test is included explicitly, its requirements / content must be included in the resolve or fail

And the flipside also being true

  • If no --variant is given (and the package has a variant), a suitable resolve using any variant is returned

I'm considering also adding behavior such as "if no test names are given, not all of the default tests are guaranteed to be part of the resolve". Because it could be that not all tests are resolvable and maybe variant "0" resolves 2/4 tests but variant 1 resolves 3/4 tests. In either case, you're missing at least one test but at least you get a resolve back. The logic does not do this currently, but that may be a consideration. Maybe people will think that's a good idea.

Special consideration is given for "on_variants". For example if 2+ tests have
a common variant that resolves and the user doesn't specify an explicit
--variant, a matching resolve is returned. However if they do specify
--variant and it is invalid, the resolve conflicts and fails.

What do you think of this behavior? I can make a PR with the changes as-is but I figured it'd be better to discuss the exact behavior in a discussion thread prior to opening a PR, in case others have suggestions.

@nerdvegas
Copy link
Contributor

Generally I think this is fine, but there's a caveat:

You can't explicitly select variants. Even if you specifically know the variant index you want, rez just doesn't work that way (yet). You can only influence the variant it selects by changing the request. rez-test already has an --extra-packages arg, which you can use to do this. So, drop all the variant-specific stuff from this. Explicit variant selection is a much bigger topic than rez-test itself, we definitely don't want to be bringing that into the mix here.

@ColinKennedy
Copy link
Author

ColinKennedy commented Mar 19, 2022

Edit: Re-read your post and now I get what you were asking for. I'll use --extra-packages in place of --variant and keep all the other functionality as planned. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants