Issue 696 shell availability #747
Conversation
-add 'is_available' method to shells -update tests to run only on available shells -renamed 'shell_dependent' to more intuitive 'per_available_shell' -made a Shell method a classmethod
|
is the intention for the tests to pass or to show as skipped in the resulting selftest? |
|
You can't actually skip the test (ala unittest.skip/TestCase.skipTest). This is because the per_available_shell decorator just loops over the wrapped function - if you skip, the remaining runs in the loop would be skipped too. This may be solvable with some fancy meta programming, but IMO isn't worth the effort. Now that you mention test skipping though, I see a couple of other things that could be done better. I'll update this PR shortly with those additional fixes. |
-remove unused function
There was a problem hiding this comment.
Tested on Windows. This is a different computer than at work and I get 2 errors on rez-selftest with cmd and a bunch more on python 3.7. But I am certain this is unrelated to your PR since master seems to be be even worse. Will create separate issues for it after I verified it is not related to my super old windows machine here. Looking forward to github actions :)
|
Will test this in a couple of hours on Arch Linux, so an ultra up-to-date system with Python 2 and 3. And as I already said, we would highly benefit to start to use pytest. This would allow us to have more granular tests (parametrized) which would allow us to skip based on complex conditions. But that's probably gonna be later. Code wise for now it looks good. |
|
I'm familiar with pytest and I agree it'd be great to update the tests to use it. One note on that before I forget - IIRC vendoring pytest is a little problematic as it brings in a bunch of dependencies. But I actually think pytest may be a case that doesn't have to be vendorized. The primary reason for vendoring is to make rezification of rez itself trivial. However, since the rez API itself does not require tests to be present, then pytest isn't required to be vendored. So, I think pytest could be included via standard requirements.txt so it gets installed into the rez virtualenv. It would actually make sense to include this via a "test" extra also. If anyone wants to talk further about this then let's take it to slack/ticket rather that polluting this thread further :) |
Relevant: #696