Merge lp:~mikefletcher/mago/gnome-screenshot into lp:~mago-contributors/mago/mago-1.0

Proposed by Michael Fletcher on 2009-08-25
Status: Merged
Merged at revision: not available
Proposed branch: lp:~mikefletcher/mago/gnome-screenshot
Merge into: lp:~mago-contributors/mago/mago-1.0
Diff against target: None lines
To merge this branch: bzr merge lp:~mikefletcher/mago/gnome-screenshot
Reviewer Review Type Date Requested Status
Ara Pulido Approve on 2009-09-07
Michael Fletcher (community) Abstain on 2009-08-27
Joker Wild (community) in process test 2009-08-25 Approve on 2009-08-27
James Tatum Approve on 2009-08-26
Nagappan Alagappan Approve on 2009-08-26
Review via email: mp+10684@code.launchpad.net

Commit message

Added a basic test suite for gnome-screenshot. I followed the existing tests documented at http://testcases.qa.ubuntu.com/Applications/TakeScreenshot.

To post a comment you must log in.
Michael Fletcher (mikefletcher) wrote :

This is a basic test suite for gnome-screenshot. I followed the existings tests documented at http://testcases.qa.ubuntu.com/Applications/TakeScreenshot.

Please be gentle. This is my first attempt at merging branches with launchpad. I might have totally messed up the branch or merge.

Michael,
Were you going to use mouse event to do this I looked at the source it seemed you started to add pointer. Then you removed it this is nice I see it grabbed a screen shot in my test it works well, but just curious if you were going to use a mouse event maybe use it to create an area to grab.

Regards,
Leo Jackson
lajjr

review: Needs Information (in process test)
Michael Fletcher (mikefletcher) wrote :

Hi.

> Were you going to use mouse event to do this I looked at the source it seemed
> you started to add pointer.
gnome-screenshot has an option to include the mouse pointer when taking the screenshot. 'include_pointer' was going to be part of those tests. However I thought I should "release early, release often" so I took those unfinished parts out.

> Then you removed it this is nice I see it grabbed
> a screen shot in my test it works well, but just curious if you were going to
> use a mouse event maybe use it to create an area to grab.
I did implement the area grab (with help from Nagappan). Its in gnome.py around line 67.

Nagappan Alagappan (nagappan) wrote :

It will be nice to move the screenshot file name to take_screenshot.xml

In gnome.py line # 164-166 coordinate information are hard coded. Will this work on all test environment ? I have a dual screen monitor ;-) I recommend to use getobjectsize and then use the relative x, y coordinates.

review: Needs Fixing
Michael Fletcher (mikefletcher) wrote :

Hi.

I will put all the arguments into the .xml file.

Gnomescreenshot is asking you to select an area of your entire screen. As
long as your screen is larger that 200x200 pixels this should be fine. In
either case you need a window for getobjectsize and in this test I don't
have one.

... sent from my Android Phone

On Aug 25, 2009 3:57 PM, "Nagappan Alagappan" <email address hidden> wrote:

Review: Needs Fixing
It will be nice to move the screenshot file name to take_screenshot.xml

In gnome.py line # 164-166 coordinate information are hard coded. Will this
work on all test environment ? I have a dual screen monitor ;-) I recommend
to use getobjectsize and then use the relative x, y coordinates.

--
https://code.launchpad.net/~mikefletcher/mago/gnome-screenshot/+merge/10684You
are the owner of...

James Tatum (jtatum) wrote :

It looks good overall. The meat of the LDTP commands in mago/application/gnome.py are all wrapped in try/catch blocks to provide a helpful failure description for the log file. +1 to the comments for moving as many parameters as possible to XML.

review: Needs Fixing
James Tatum (jtatum) wrote :

Sorry, one additional note regarding style, I notice that most of the mago/application/gnome.py applications are written as ooldtp, and the proposed code for gnome-screenshot is procedural. I'm not sure if that's a factor since I'm new here myself :) Just pointing it out.

Michael Fletcher (mikefletcher) wrote :

Hi. I made all of the requested changes to the gnome-screenshot tests.

* ooldtp is being used
* try: except: should indicate where something fails
* the arguments are being specified using the xml files.

review: Resubmit
Nagappan Alagappan (nagappan) wrote :

> Hi. I made all of the requested changes to the gnome-screenshot tests.
>
> * ooldtp is being used
> * try: except: should indicate where something fails
> * the arguments are being specified using the xml files.

Am I missing something ? I could not see any new diff in this review.

Thanks

review: Needs Information
Nagappan Alagappan (nagappan) wrote :

> Am I missing something ? I could not see any new diff in this review.

jtatum gave me the link - http://bazaar.launchpad.net/~mikefletcher/mago/gnome-screenshot/revision/109

Its nice to see the exceptions are caught in each and every line. But this will make the script writer job more difficult !?!

Thanks

review: Approve
James Tatum (jtatum) wrote :

+1 - that is a lot of exceptions :) It can always be condensed later of course. I think the balance in e.g. seahorse in gnome.py is pretty good.

review: Approve
Michael Fletcher (mikefletcher) wrote :

Sorry about the change-of-status spam. I had heard that changing the status of the merge proposal would regenerate the diff but it didn't work.

Michael Fletcher (mikefletcher) wrote :

I think the exception handling is a bit much. However it is consistent with
the rest of the code and i'm not interested in rocking.

The ldtp exception says
    u'Unable to find object name in application map'

And the exceptions we raise says
    'Window dlgSaveScreenshot did not appear'

I think a good solution would be to add information to the ldtp exceptions
so that the mago exception handling is superfluous. But I wont mention it
again until I have written a patch :).

110. By Michael Fletcher on 2009-08-26

Updated test to work with 9.10. It will now fail with 9.04.

111. By Michael Fletcher on 2009-08-26

Fixed silly mistake. I should have run the tests before I commited.

With changes in place very good.

review: Approve (in process test)
review: Abstain
Javier Collado (javier.collado) wrote :

> Sorry about the change-of-status spam. I had heard that changing the status
> of the merge proposal would regenerate the diff but it didn't work.

I also noticed this in other reviews.

It seems like this is known problem that is going to be solved:
https://bugs.launchpad.net/launchpad-code/+bug/338002

Look at comment #1 for a tool that is supposed to help to workaround it.

112. By Michael Fletcher on 2009-09-02

Reformatted and updated the documentation for epydoc.

Michael Fletcher (mikefletcher) wrote :

I pushed some fixes to the documentation strings so they get picked
up by epydoc.

Reminder: The diff does not get updated so you will have to look at
the individual changes (changes 106 -> 112).

Ara Pulido (ara) wrote :

> I think the exception handling is a bit much. However it is consistent with
> the rest of the code and i'm not interested in rocking.
>
>
> The ldtp exception says
> u'Unable to find object name in application map'
>
> And the exceptions we raise says
> 'Window dlgSaveScreenshot did not appear'
>
>
> I think a good solution would be to add information to the ldtp exceptions
> so that the mago exception handling is superfluous. But I wont mention it
> again until I have written a patch :).

I agree with you, Michael, although I was the one that started the try/catch madness, I never liked it myself.

Ara Pulido (ara) wrote :

I think it is ready to be merged. I am waiting for Michael's reply to an email I sent him, before commiting

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added directory 'gnome-screenshot'
2=== added file 'gnome-screenshot/take_screenshot.py'
3--- gnome-screenshot/take_screenshot.py 1970-01-01 00:00:00 +0000
4+++ gnome-screenshot/take_screenshot.py 2009-08-25 18:33:48 +0000
5@@ -0,0 +1,48 @@
6+import os
7+from mago.test_suite.gnome import GnomeScreenshotTestSuite
8+
9+class TakeScreenshot(GnomeScreenshotTestSuite):
10+ def grab_desktop(self, delay=5, file_name='baz.png'):
11+ self.application.open()
12+ self.application.take_whole_desktop_screenshot(delay)
13+
14+ self.application.save_to_file(file_name)
15+
16+ file_path = os.getenv('HOME') + '/' + file_name
17+
18+ if os.access(file_path, os.F_OK):
19+ os.remove(file_path)
20+ else:
21+ raise AssertionError('Screenshot ' + file_path + ' was not created')
22+
23+ def grab_window(self, delay=5, file_name='baz.png'):
24+ self.application.open()
25+
26+ self.application.take_current_window_screenshot(delay)
27+
28+ self.application.save_to_file(file_name)
29+
30+ file_path = os.getenv('HOME') + '/' + file_name
31+
32+ if os.access(file_path, os.F_OK):
33+ os.remove(file_path)
34+ else:
35+ raise AssertionError('Screenshot ' + file_path + ' was not created')
36+
37+ def grab_area(self, delay=5, file_name='baz.png'):
38+ self.application.open()
39+
40+ self.application.grab_selected_area(delay)
41+
42+ self.application.save_to_file(file_name)
43+
44+ file_path = os.getenv('HOME') + '/' + file_name
45+
46+ if os.access(file_path, os.F_OK):
47+ os.remove(file_path)
48+ else:
49+ raise AssertionError('Screenshot ' + file_path + ' was not created')
50+
51+if __name__ == "__main__":
52+ test = TakeScreenshot()
53+ test.run()
54
55=== added file 'gnome-screenshot/take_screenshot.xml'
56--- gnome-screenshot/take_screenshot.xml 1970-01-01 00:00:00 +0000
57+++ gnome-screenshot/take_screenshot.xml 2009-08-25 18:33:48 +0000
58@@ -0,0 +1,25 @@
59+<?xml version="1.0"?>
60+<suite name="take screenshot">
61+ <class>take_screenshot.TakeScreenshot</class>
62+ <description>
63+ Tests which verify the Take Screenshot functionality.
64+ </description>
65+ <case name="Grab Desktop">
66+ <method>grab_desktop</method>
67+ <description>
68+ Grab a screenshot of the desktop after a delay.
69+ </description>
70+ </case>
71+ <case name="Grab Window">
72+ <method>grab_window</method>
73+ <description>
74+ Grab a screenshot of the current window after a delay.
75+ </description>
76+ </case>
77+ <case name="Grab Area">
78+ <method>grab_area</method>
79+ <description>
80+ Grab a screenshot of the select area after a delay.
81+ </description>
82+ </case>
83+</suite>
84
85=== modified file 'mago/application/gnome.py'
86--- mago/application/gnome.py 2009-07-10 09:24:57 +0000
87+++ mago/application/gnome.py 2009-08-25 18:33:48 +0000
88@@ -4,9 +4,98 @@
89 The gnome module provides wrappers for LDTP to make the write of Gnome tests easier.
90 """
91 import ooldtp
92-import ldtp
93+import ldtp
94 from .main import Application
95
96+class GnomeScreenshot(Application):
97+ LAUNCHER = 'gnome-screenshot'
98+ LAUNCHER_ARGS = ['--interactive']
99+ CLOSE_TYPE = 'button'
100+ CLOSE_NAME = 'btnCancel'
101+
102+ WINDOW = 'dlgTakeScreenshot'
103+
104+ GRAB_AFTER_DELAY_FIELD = 'sbtnGrabafteradelayof'
105+ GRAB_THE_WHOLE_DESKTOP_RADIO = 'rbtnGrabthewholedesktop'
106+ GRAB_THE_CURRENT_WINDOW_RADIO = 'rbtnGrabthecurrentwindow'
107+ GRAB_A_SELECTED_AREA = 'rbtnGrabaselectedarea'
108+
109+ TAKE_SCREENSHOT_BUTTON = 'btnTakeScreenshot'
110+
111+ SAVE_FILE_WINDOW = 'dlgSaveScreenshot'
112+ SAVE_FILE_BUTTON = 'btnSave'
113+ SAVE_FILE_NAME_TEXT = 'txtName'
114+ SAVE_FILE_FOLDER_DROPDOWN = 'cbSaveFile'
115+
116+ def __init__(self):
117+ Application.__init__(self)
118+
119+ """
120+ Complete the "Take Screenshot" dialog box by choosing whole desktop and
121+ clicking Take Screenshot.
122+
123+ You must have started the application already.
124+ """
125+ def take_whole_desktop_screenshot(self, delay):
126+ ldtp.click(self.WINDOW, self.GRAB_THE_WHOLE_DESKTOP_RADIO)
127+
128+ ldtp.setvalue (self.WINDOW, self.GRAB_AFTER_DELAY_FIELD, delay)
129+
130+ ldtp.click(self.WINDOW, self.TAKE_SCREENSHOT_BUTTON)
131+
132+ ldtp.waittillguinotexist(self.WINDOW)
133+
134+ """
135+ Complete the "Take Screenshot" dialog box by choosing current window and
136+ clicking Take Screenshot.
137+
138+ You must have started the application already.
139+ """
140+ def take_current_window_screenshot(self, delay):
141+ ldtp.click(self.WINDOW, self.GRAB_THE_CURRENT_WINDOW_RADIO)
142+
143+ ldtp.setvalue (self.WINDOW, self.GRAB_AFTER_DELAY_FIELD, delay)
144+
145+ ldtp.click(self.WINDOW, self.TAKE_SCREENSHOT_BUTTON)
146+
147+ ldtp.waittillguinotexist(self.WINDOW)
148+
149+ """
150+ Complete the "Take Screenshot" dialog box by choosing selected area and
151+ clicking Take Screenshot.
152+
153+ You must have started the application already.
154+ """
155+ def grab_selected_area(self, delay):
156+ ldtp.click(self.WINDOW, self.GRAB_A_SELECTED_AREA)
157+
158+ ldtp.setvalue (self.WINDOW, self.GRAB_AFTER_DELAY_FIELD, delay)
159+
160+ ldtp.click(self.WINDOW, self.TAKE_SCREENSHOT_BUTTON)
161+
162+ ldtp.waittillguinotexist(self.WINDOW)
163+
164+ ldtp.generatemouseevent(100,100,'b1p')
165+ ldtp.generatemouseevent(200,200,'abs')
166+ ldtp.generatemouseevent(200,200,'b1r')
167+
168+ """
169+ Complete the save to file dialog. You must have completed a grab_ already
170+ """
171+ def save_to_file(self, filename):
172+ ldtp.waittillguiexist(self.SAVE_FILE_WINDOW)
173+
174+ ldtp.settextvalue(self.SAVE_FILE_WINDOW, self.SAVE_FILE_NAME_TEXT, filename)
175+
176+ # There is a bug in gnome-screenshot that prevents the combobox
177+ # name from being used. There is only one combo box so its easy to
178+ # select it by index.
179+ ldtp.comboselectindex(self.SAVE_FILE_WINDOW, 'cbo#0', 0)
180+
181+ ldtp.click(self.SAVE_FILE_WINDOW, self.SAVE_FILE_BUTTON)
182+
183+ ldtp.waittillguinotexist(self.SAVE_FILE_WINDOW)
184+
185 class Seahorse(Application):
186 """
187 Seahorse manages the Seahorse application.
188
189=== modified file 'mago/application/main.py'
190--- mago/application/main.py 2009-07-29 11:14:24 +0000
191+++ mago/application/main.py 2009-08-20 02:40:51 +0000
192@@ -10,13 +10,16 @@
193 Superclass for the rest of the applications
194
195 Constants that should be defined in the classes that inherit from this class
196- LAUNCHER: Argument to be passed when launching the application through ldtp.launchapp
197+ LAUNCHER: Command to launching the application through ldtp.launchapp
198+ LAUNCHER_ARGS: Arguments to pass when launching the application through
199+ ldtp.launchapp.
200 WINDOW: Top application frame pattern using ldtp syntax
201 CLOSE_TYPE: Close object type (one of 'menu' or 'button')
202 CLOSE_NAME: Close object name
203 """
204 CLOSE_TYPE = 'menu'
205 CLOSE_NAME = 'mnuQuit'
206+ LAUNCHER_ARGS = ['']
207 WINDOW = ''
208 TOP_PANEL = 'frmTopExpandedEdgePanel'
209
210@@ -68,7 +71,7 @@
211
212 """
213 self._enable_a11y(True)
214- ldtp.launchapp(self.LAUNCHER)
215+ ldtp.launchapp(self.LAUNCHER, arg=self.LAUNCHER_ARGS)
216 self._enable_a11y(False)
217
218 ldtp.wait(2)
219
220=== modified file 'mago/test_suite/gnome.py'
221--- mago/test_suite/gnome.py 2009-05-27 10:45:38 +0000
222+++ mago/test_suite/gnome.py 2009-08-25 18:33:48 +0000
223@@ -4,7 +4,10 @@
224 """
225 import ldtp, ooldtp
226 from .main import SingleApplicationTestSuite
227-from ..application.gnome import Application, Seahorse, GEdit
228+from ..application.gnome import Application, GnomeScreenshot, Seahorse, GEdit
229+
230+class GnomeScreenshotTestSuite(SingleApplicationTestSuite):
231+ APPLICATION_FACTORY = GnomeScreenshot
232
233 class SeahorseTestSuite(SingleApplicationTestSuite):
234 """

Subscribers

People subscribed via source and target branches

to status/vote changes: