Feature #8688
Write regression test for GNOME screenshot
80%
Subtasks
History
#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.
#4 Updated by intrigeri 2015-01-23 10:52:10
> Lastly, I think it would be great to actually test GNOME Screenshot too
+1 for behavior testing :)
#5 Updated by kytv 2015-02-08 17:50:41
anonym wrote:
> 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.
Hm. I’m not seeing what I’m doing wrong.
With the following:
Then /^GNOME Screenshot is configured to save files to the live user's home directory$/ do
next if @skip_steps_while_restoring_background
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.")
@screen.type(Sikuli::Key.PRINTSCREEN)
screenfile = @vm.execute("ls -l #{home}").stdout.chomp
puts "created #{screenfile}"
end
I get
Scenario: GNOME Screenshot has a sane default save directory # features/checks.feature:13
calling as root: true
call returned: [0, "", ""]
[log] CLICK on (1024,384)
calling as root: /sbin/ifconfig eth0 | grep -q 'inet addr'
call returned: [1, "", ""]
calling as root: gsettings get org.gnome.gnome-screenshot auto-save-directory
call returned: [0, "'file:///home/amnesia'\n", ""]
[log] TYPE ""
calling as root: ls -l /home/amnesia
call returned: [0, "total 0\ndrwx------ 2 amnesia amnesia 80 Feb 8 16:57 Desktop\n", ""]
Then GNOME Screenshot will save files to the live user's home directory # features/step_definitions/checks.rb:99
created total 0
drwx------ 2 amnesia amnesia 80 Feb 8 16:57 Desktop
The screenshot is not being created with Sikuli directed to press PrintScreen
, but if I press PrintScreen
myself the screenshot is created as expected.
#6 Updated by intrigeri 2015-02-08 17:53:12
>
> @screen.type(Sikuli::Key.PRINTSCREEN)
> screenfile = @vm.execute("ls -l #{home}").stdout.chomp
>
I think you should wait between these two steps: the screenshot won’t be created as soon as the key is released.
#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}.”)
- We need to pass
-maxdepth 1
tofind
so we don’t find stuff in any sub-directory. - 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