Re: [Tails-dev] Please review'n'merge test/fix-persistence-c…

Borrar esta mensaxe

Responder a esta mensaxe
Autor: intrigeri
Data:  
Para: The Tails public development discussion list
Temas novos: Re: [Tails-dev] Please review'n'merge test/fix-persistence-checks
Asunto: Re: [Tails-dev] Please review'n'merge test/fix-persistence-checks
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!