Re: [Tails-dev] [review'n'merge:1.3] feature/5525-sandbox-we…

Nachricht löschen

Nachricht beantworten
Autor: intrigeri
Datum:  
To: The Tails public development discussion list
Betreff: Re: [Tails-dev] [review'n'merge:1.3] feature/5525-sandbox-web-browser
Hi,

anonym wrote (10 Feb 2015 15:29:58 GMT) :
> Here's a two suggested improvements (but not blockers):


Thanks!

>> +When /^I press the "([^"]+)" key$/ do |key|
>> +  next if @skip_steps_while_restoring_background
>> +  case key
>> +  when "ENTER"
>> +    @screen.type(Sikuli::Key.ENTER)
>> +  else
>> +      raise "unsupported key #{key}"
>> +  end
>> +end


> I'd suggest the following so a special case doesn't have to be added for
> each key:


Looks good. I admit I was too lazy to learn Ruby's meta-programming
features, although this idea did occur to me. Going to test it locally
and add this to the branch.

>> +When /^(no|\d+) application(?:s?) (?:is|are) playing audio(?:| after (\d+) seconds)$/ do |nb, wait_time|
>> + next if @skip_steps_while_restoring_background
>> + nb = 0 if nb == "no"
>> + sleep wait_time.to_i if ! wait_time.nil?
>> + assert_equal(nb.to_i, pulseaudio_sink_inputs)
>> +end


> Why not use `try_for` instead of `sleep`, and s/after/within/ or
> s/after/after at most/ in the last optional part of the step name? That
> way the test succeeds as fast as possible and its more consistent with
> how we generally deal with waiting times.


Sure, this would be much nicer. I've spent 1-2 hours trying to make
this work when writing this test, and I failed, so in the end
I implemented it this way. I won't try again, but if someone manages
to make it work without spending hours on it, I'll be very happy :)

> Lastly, one scenario fails consistently for me:


>   Scenario: Importing an OpenPGP key from website
> [...]
>     Then I see "KeyImportedNotification.png" after at most 10 seconds
>       FindFailed: can not find KeyImportedNotification.png on the screen.


> The failure screen shot still shows the "Save file" dialog so it seems
> the ENTER key press was lost. I guess we have to click the dialog's OK
> button instead.


OK, will look into it right now.

> Lastly, regarding torbrowser-AppArmor-profile.patch, I think it'd be
> helpful to keep have it also add comments about why we do each change
> (unless they are completely obvious, e.g. dealing with differing paths).
> Currently quite a lot can be seen through the patch's Git history, but
> it's not very convenient. What do you think?


I'm unsure. We've decided to maintain this AppArmor profile in
a dedicated Git repo, so for now I'm documenting each change (except
the initial largish patch import) atomically there. I generally
dislike writing basically the same thing in a code comment, and in the
commit message that describes the change, even if I sometimes do it
when I think a comment is really needed.

Let's say I'll add comments wherever I think it makes sense when
working on #8007 ("Self-audit our AppArmor profiles"), OK?

Cheers!
--
intrigeri