On 06/02/15 19:53, intrigeri wrote:
> Hi,
>
> this is an initial iteration for reviewing my implementation of #5525.
>
> That's a complex topic, so I don't expect it to be merged as-is (who
> knows!) => a *quick* initial review would be much appreciated, so that
> I have time to fix whatever needs to be in time for the feature freeze
> in a few days.
I've tested it quite a lot, and I'm almost ready to merge it. See below.
> Note that I've just sent a call for testing on tails-testers@ about it.
We'll get more testing with the 1.3 RC.
Here's a two suggested improvements (but not blockers):
> +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:
When /^I press the "([^"]+)" key$/ do |key|
next if @skip_steps_while_restoring_background
- case key
- when "ENTER"
- @screen.type(Sikuli::Key.ENTER)
+ if Sikuli::Key.method_defined?(key)
+ @screen.type(eval("Sikuli::Key.#{key}"))
else
- raise "unsupported key #{key}"
+ raise "unsupported key #{key}"
end
end
Also note the indentation fix.
> +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.
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.
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?
Cheers!
