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.]
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!
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.
> + foreach my $x (Tails::Persistence::Configuration::Presets->new()->all) {
s/x/preset/ ?
> + 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.
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, 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?
Cheers,
--
intrigeri
