Project

General

Profile

Feature #8688

Feature #8540: Write regression tests and tests for new features

Write regression test for GNOME screenshot

Added by intrigeri about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
01/13/2015
Due date:
% Done:

80%

Feature Branch:
kytv:test/8688-gnome-screenshot
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

The bug that was fixed is #8087.

Associated revisions

Revision 177ae4d3
Added by Tails developers almost 5 years ago

Merge remote-tracking branch 'kytv/test/8688-gnome-screenshot' into testing

Fix-committed: #8688

History

#1 Updated by kytv about 5 years ago

  • Status changed from Confirmed to In Progress

#2 Updated by kytv about 5 years ago

  • 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 almost 5 years ago

  • 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 almost 5 years ago

Lastly, I think it would be great to actually test GNOME Screenshot too

+1 for behavior testing :)

#5 Updated by kytv almost 5 years ago

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 almost 5 years ago

>   @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 almost 5 years ago

  • 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 almost 5 years ago

  • 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 #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 #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 almost 5 years ago

  • 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 #8885, split & fixed up, and force pushed.

#10 Updated by intrigeri almost 5 years ago

  • Assignee changed from anonym to intrigeri

Reviewing and improving it.

#11 Updated by intrigeri almost 5 years ago

  • 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 almost 5 years ago

  • 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 almost 5 years ago

  • 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 almost 5 years ago

  • Status changed from In Progress to 11
  • % Done changed from 80 to 100

Applied in changeset commit:655a6d1a93fad90397da82fc9f3b31a803be9a0e.

#15 Updated by anonym almost 5 years ago

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

Merged!

#16 Updated by BitingBird almost 5 years ago

  • Status changed from 11 to Resolved

Also available in: Atom PDF