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

Delete this message

Reply to this message
Autore: intrigeri
Data:  
To: The Tails public development discussion list
Vecchi argomenti: Re: [Tails-dev] Please review'n'merge test/fix-persistence-checks
Oggetto: Re: [Tails-dev] Please review'n'merge test/fix-persistence-checks
(FTR that's #5465.)

intrigeri wrote (27 Jul 2013 16:59:39 GMT) :
> 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.


Done in that branch.

> 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 think I'll just revert this part unless someone objects.

> I'm marking #5465 as "dev needed", and reassigning to anonym.


I'll bring this branch to completion.

Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc