Hi,
anonym wrote (26 Jul 2013 12:47:46 GMT) :
> branch: test/fix-persistence-checks
I've had a look. This changeset generally looks generally good to me
(though I haven't tested it yet, mostly because I don't expect my test
setup to be able to go as far as testing these steps...).
Nitpicking follows.
> + return @vm.execute("test -e #{persistence_state_file}").success? &&
> + @vm.execute(". #{persistence_state_file}; " +
> + "test $TAILS_PERSISTENCE_ENABLED = true").success?
Due to the lack of quotes around $TAILS_PERSISTENCE_ENABLED, if the
tested condition is not verified (e.g. persistence_state_file is
missing empty or something), then I guess that the *shell* will error
out due to a syntax error, which IMHO is poor reporting from a test
suite: in error conditions, I think a test suite should tell us that
what we're testing is not verified, e.g. something like
"$TAILS_PERSISTENCE_ENABLED is not set to true as it is supposed to
be", instead of just erroring out with "parse error: condition
expected: =" because it could actually *not* perform a test.
What do you think?
In the same spirit, perhaps '&&' could be used instead of ';' to
chain commands in there.
To end with, I fail to see the rationale to split the "all persistent
directories are mounted" step out of "persistence is enabled". I think
it's the kind of low-level implementation detail that makes it harder
to read and maintain features, and that should instead be hidden in
step definitions. It's probably not in these terms that a Tails user
would express their experience of the Tails persistence feature.
What do you think?
I'm marking #5465 as "dev needed", and reassigning to anonym.
Cheers!
