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
