Tails

#1 Updated by kytv 2015-01-15 20:54:36

• Status changed from Confirmed to In Progress

#2 Updated by kytv 2015-01-16 11:03:46

• Assignee changed from kytv to anonym
• % Done changed from 0 to 40
• QA Check set to Ready for QA
• Feature Branch set to kytv:test/8688-gnome-screenshot

#3 Updated by anonym 2015-01-23 09:47:23

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

Here’s an initial review:

> +Then /^GNOME screenshot will save files to the correct location$/ do

Given what this step actually does it should be called “GNOME Screenshot is configured to save files to the correct location”, since we’re not testing the actual functionality. Also “the correct location” is a bit vague, and I think I’d prefer the more specific “the live user’s home folder”. That way a human could read the scenario and actually know what was expected.

The code looks good, except for one tiny detail; assert_equal and friends take the expected value as the first parameter. This is really only relevant when an assertion failure message is not given, since it then will create a generic one which says something like “expected $1 but got $2”, but I think it’s good style to always follow that convention.

Lastly, I think it would be great to actually test GNOME Screenshot too (who knows, maybe some future GNOME release will change the dconf key?), e.g.:

  Scenario: GNOME Screenshot has a sane default save directory
    Given GNOME Screenshot is configured to save files to the live user's home folder
    When I press the Print Screen keyboard button
    Then a screenshot is saved in the live user's home folder

Hint: Sikuli’s JavaDoc is a good resource, where you could find org.sikuli.script.Key.PRINTSCREEN which would become Sikuli::Key.PRINTSCREEN thanks to our Ruby conversion.

#7 Updated by kytv 2015-02-08 22:45:04

• Assignee changed from kytv to anonym
• % Done changed from 40 to 50
• QA Check changed from Dev Needed to Ready for QA

intrigeri wrote:
> > […]
>
> I think you should wait between these two steps: the screenshot won’t be created as soon as the key is released.

Yes, cheers. I went out for a bit and this came to mind while I was away. Grr.

Fixed up, rebased against devel and force pushed.

#8 Updated by anonym 2015-02-11 10:29:33

• Assignee changed from anonym to kytv
• QA Check changed from Ready for QA to Dev Needed

> +Then /^GNOME Screenshot is configured to save files to the live user’s home directory$/ do

IMHO this step does too many things, and should be split like I suggested in Feature #8688#note-3.

> + next if skip_steps_while_restoring_background > + screen.type(Sikuli::Key.PRINTSCREEN)

In devel we now have the I press press the "blah" key step, which can be extended to also handle PRINTSCREEN. Even better, you lift my patch from Feature #8885 so that it doesn’t have to be extended ever again.

Also, why pressing PRINTSCREEN here, then doing some unrelated stuff (below), and then finally looking if the action from the key press triggered? If it was in hope that the unrelated code would take enough time for the file to be created, that’s not reliable.

> + home = “/home/#{$live_user}”
> + save_path = @vm.execute(“gsettings get org.gnome.gnome-screenshot auto-save-directory”).stdout.chomp.tr(“’”,“”)
> + expected = “file://#{home}”
> + assert_equal(expected, save_path, “The GNOME screenshot auto-save-directory is not set correctly.”)

IMHO this is the only code that belong in this step, given the name (the key is the “configured” part).

> + sleep 1
> + screenshot = @vm.execute(“find #{home} -name ‘Screenshot*.png’”).stdout.chomp
> + assert(screenshot.length > 0, “Screenshot not created in #{home}.”)

1. We need to pass -maxdepth 1 to find so we don’t find stuff in any sub-directory.
2. Let’s not introduce any more static sleeping when it can be avoided. Instead use try_for, e.g.:

   try_for(10, :msg => "Screenshot not created in #{home}") do
     !@vm.execute("find #{home} -maxdepth 1 -name 'Screenshot*.png'").stdout.empty?
   end

10 seconds is of course a bit arbitrarily chosen. :)

> + if not screenshot.nil?
> + puts “Screenshot saved to the file ‘#{screenshot}’.”
> + end

IMHO this can be killed.

> +end

#9 Updated by kytv 2015-02-11 13:52:02

• Assignee changed from kytv to anonym
• QA Check changed from Dev Needed to Ready for QA

Perhaps “the 3rd time’s a charm”? :(

Rebased against devel, patch lited from Feature #8885, split & fixed up, and force pushed.

#10 Updated by intrigeri 2015-02-12 14:27:32

• Assignee changed from anonym to intrigeri

Reviewing and improving it.

#11 Updated by intrigeri 2015-02-12 14:30:26

• Assignee changed from intrigeri to anonym
• % Done changed from 50 to 60
• Feature Branch changed from kytv:test/8688-gnome-screenshot to test/8688-gnome-screenshot

Improved a bit, ready for QA again.

#12 Updated by anonym 2015-02-17 17:56:30

• Assignee changed from anonym to kytv
• % Done changed from 60 to 70
• QA Check changed from Ready for QA to Dev Needed

> Then /^there is no screenshot in the live user’s home directory$/ do
> next if skip_steps_while_restoring_background > home = "/home/#{$live_user}" > vm.execute(“find ‘#{home}’ -name ‘Screenshot*.png’ -maxdepth 1”).stdout.empty?
> end

This step do what it’s supposed to — an exception must be thrown for a step to fail. Add the appropriate assertion with an appropriate failure message and the branch can be merged, cause the rest looks good!

#13 Updated by kytv 2015-02-17 19:12:48

• Assignee changed from kytv to anonym
• % Done changed from 70 to 80
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from test/8688-gnome-screenshot to kytv:test/8688-gnome-screenshot

I added an assert to intrigeri’s step; for whatever reason I didn’t think to check that the file didn’t exist yet.

Anyhow, assert and appropriate message added. :)

#14 Updated by Tails 2015-02-18 09:10:25

• Status changed from In Progress to Fix committed
• % Done changed from 80 to 100

Applied in changeset commit:655a6d1a93fad90397da82fc9f3b31a803be9a0e.

#15 Updated by anonym 2015-02-18 09:12:53

• Assignee deleted (anonym)
• % Done changed from 100 to 80
• QA Check changed from Ready for QA to Pass

Merged!

#16 Updated by BitingBird 2015-02-24 22:45:01

• Status changed from Fix committed to Resolved