On 31/10/14 14:49, intrigeri wrote:
> Hi,
>
> I could not find this pull request in the list's archives, so let's
> emulate it.
>
> [Off-topic: note that we still have *not* made it so we can reasonably
> skip sending pull requests here (namely: providing, documenting and
> advertising a way to follow pull requests and merges via Redmine --
> that is, I guess, "QA check" field value changing to "Ready for QA" or
> to "Fix committed"). Anyone willing to skip sending pull requests
> here, please make sure we have a ticket about the missing pieces, and
> possibly implement it.]
Sorry! I just hate the duplicate communication channel overhead and
confusion.
> I had a first quick look at the branch. I only have minor comments
> regarding implementation details, and overall it really looks good
> (and will make our life easier :) Congrats!
Thanks for the review! Also, I'm sorry for letting this slide for so long...
> I've also asked Kill Your TV if they wanted to have a look and comment
> too, in order to get them started with the automated test suite.
>
>> + presets = @vm.execute_successfully("perl -e '#{script}'").stdout.chomp.split("\n")
>
> Better use "perl -E", and get rid of "use feature qw(say)" in
> the script.
Fixed!
>> + foreach my $x (Tails::Persistence::Configuration::Presets->new()->all) {
>
> s/x/preset/ ?
Fixed!
>> + source_str = options.find { |option| /^source=/.match option }
>> + assert_not_nil source_str
>
> This will break if we add a preset that has no source= option, right?
> If we really want to commit not to do that ever, I wonder if we should
> maybe enforce this in the t-p-s code.
Right, fixed!
> To end with, the code added by commit 1f2cdd8 ("Verify that all
> available presets were enabled") feels a bit redundant, as we're
> testing the effects of what's being checked here in the following
> scenarios. I understand that it allows us to detect breakage earlier
> and more precisely when running the test suite, and was possibly
> useful while developing the previous commit,
IIRC the latter was the case, and I though "hey, why not add this since
it actually was useful?". :)
> but I'm not sure if it's
> really worth having more code to maintain there (e.g.
> the /media/TailsData path will need to be adapted for Jessie, to start
> with). A compromise might be to compare the number of non-empty lines
> in persistence.conf with persistent_dirs.size: we still have to
> maintain some additional and mostly redundant code, but at least we
> don't have to maintain persistence.conf parsing code in yet another
> place. What do you think?
I agree (and it's verified by commit ce0cef9 enabling something that
would make that code incorrect) and do not think the "compromise" you
propose is needed either, since it will be tested later any way. I
simply reverted commit 1f2cdd8.
Cheers!
