Added check of expanded StageComposite property
#11661
Conversation
tgamblin
changed the title
expanded StageComposite property (see #11647).expanded StageComposite property
Sure. |
tldahlgren
force-pushed
the
tests/add-composite-expanded-check
branch
from
c2f9a9a
to
fdb0c42
Compare
tldahlgren
force-pushed
the
tests/add-composite-expanded-check
branch
from
ac2d2c5
to
0f1fba9
Compare
There was a problem hiding this comment.
I have a few requests:
- Update exception handling and new exception classes (more in comments)
- Update parameterization of some tests (more in comments)
- Retrieve expected test values from object properties rather than using static strings (more on this in comment for
test_location_build_dir)
lib/spack/spack/cmd/location.py
Outdated
| @@ -69,7 +68,7 @@ def location(parser, args): | |||
| elif args.env: | |||
| path = spack.environment.root(args.env) | |||
| if not os.path.isdir(path): | |||
| tty.die("no such environment: '%s'" % args.env) | |||
| raise BadEnvironmentError("No such environment: '%s'" % args.env) | |||
There was a problem hiding this comment.
I know I made a comment here suggesting that we should replace tty.die invocations with SpackError subclasses, but I meant for core code. This code is in a command -- where it's fine (and probably clearer) to use tty.die() instead of introducing many different exception types. Commands are really what tty.die() is for. You can think of them as just an extended main() method, where calling exit() is fine. They're not library code - they're supposed to be simple, and if things get complex in the commands, we try to push the complexity into the core so that their logic stays simple.
The only thing that invokes a command besides the framework in spack.main is tests, and for command tests, it's sufficient to just do with pytest.raises(SystemExit):. The one I complained about here was a method of DIYStage (a core class) calling tty.die(). Sorry for confusing things.
Basically, I don't think we need to go overboard on exception types here. My general rule of thumb is that we shouldn't define new exception types in the commands, because it clutters up the command logic for not a lot of gain. I'd just revert this change and the others in cmd/location.py.
There was a problem hiding this comment.
I suspected as such wrt tty.die versus exceptions but it's good to get confirmation.
There was a problem hiding this comment.
Thankfully I found the fail_on_error option so I could still check the unique output message instead of SpackCommandError's standard Command exited with code 1: location(<args>).
|
|
||
| @pytest.mark.parametrize('specs,expected', [ | ||
| ([], "You must supply a spec."), | ||
| (['spec1', 'spec2'], "Too many specs. Supply only one.")]) |
There was a problem hiding this comment.
This can just stay the way it is for now, but in general I think we don't want to be too specific about what we check for. It can end up being brittle. I think this is fine for now -- if it breaks we'll make it less specific.
There was a problem hiding this comment.
I debated myself on the specificity because there is a trade-off, especially wrt error messages.
Thanks.
The "spack location" command was previously untested. This also adds a check to ensure that composite Stages can report whether they were expanded (this property was previously only recorded in Stage but not in CompositeStage).
The "spack location" command was previously untested. This also adds a check to ensure that composite Stages can report whether they were expanded (this property was previously only recorded in Stage but not in CompositeStage).
Add test per discussion in #11647