Merge into trunk : autopilot-indicator-page-title-matches-widget : Code : Unity 8

Reviewer	Review Type	Date Requested	Status
Michał Sawicz		2013-11-27	Approve on 2014-04-10
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-08
Leo Arias (community)	code review		Approve on 2014-04-01
Nick Dedekind (community)		2013-11-27	Approve on 2014-01-13
Albert Astals Cid (community)			Needs Information on 2014-01-09
Christopher Lee (community)		2013-11-27	Approve on 2013-12-04
Review via email: mp+196991@code.staging.launchpad.net

Revision history for this message

Allan LeSage (allanlesage) wrote on 2013-11-27:

#

Some questions for reviewers:

dednick, can you advise on the best way to assert an indicator page is open? Concerns ll. 59 and 69.

veebers, does the 'scenario' form of this added test make sense, or should it be mashed into the original IndicatorTestCase ?

veebers, can you comment on the necessity of get_center utility function when using a Touch Pointer object? I see that there are utilities for pressing the centers of buttons, etc., but in any case I'll need an origin point to perform a drag, correct?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-28:

#

FAILED: Continuous integration, rev:503
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget/+merge/196991/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1763/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Christopher Lee (veebers) wrote on 2013-11-28:

#

Hi Allan, just a couple of comments.

I would make a DefaultIndicatorWidget emulator,

  - to this add the swipe_to_open and swipe_to_close methods
  - perhaps also add get_center for now (although as mentioned there is a better
    place).
    -> Yes you're correct that you'll need to provide the coords, unlike
       click_object which determines the center itself.

Then there is no need for an IndicatorBaseTestCase class as the helpers are part
of the emulator/proxy that needs them.

line:58, 61 cleanup not needed as drag is a "finger down, move and finger up"
operation.
line:46-49: not needed (as per above)

line:120 -> should have the skip within the setup (not decorated).
i.e.
def setUp(self):
if model() == "Desktop":
self.skipTest("Test cannot be run on the desktop.")

line:126 -> Docstring
The docstring should be descriptive not narrative.
"Swiping open an indicator must show the correct title."

line:140
  This assert is not needed, if there is nothing found for the criteria
  (i.e. type and title text in this case) wait_select_single will raise an
  exception (thus failing the test.)

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-29:

#

FAILED: Continuous integration, rev:503
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget/+merge/196991/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1774/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-11-29:

#

FAILED: Continuous integration, rev:503
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget/+merge/196991/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1781/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-03:

#

FAILED: Continuous integration, rev:506
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget/+merge/196991/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1809/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-04:

#

FAILED: Continuous integration, rev:508
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget/+merge/196991/+edit-commit-message

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1810/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Christopher Lee (veebers) wrote on 2013-12-04:

#

The acceptance tests look good to me.

Not happy about the "# TODO: assert that the indicator page closed" but as they're not required for this I will approve.

Please file a bug against the tests to fix the TODOs.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-04:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1811/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-04:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1812/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2013-12-04:

#

/tmp/buildd/unity8-7.84+14.04.20131128.2/tests/autopilot/unity8/indicators/emulators/widget.py: bad whitespace in line 31

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-05:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1836/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Leo Arias (elopio) wrote on 2013-12-06:

#

53 +

A small pita comment, I don't like that space :)

64 + self.globalRect[0]+int(self.globalRect[2]/2),
65 + self.globalRect[1]+int(self.globalRect[3]/2)

You now can do globalRect.x, globalRect.y, globalRect.height, globalRect.width. That makes it clearer.

77 + # TODO: assert that the indicator page opened

A nice thing that might be useful in the future is signing the comments like that, using -- alesage - 2013-12-06.

153 + self.assertIsNotNone(widget)
189 + self.assertIsNotNone(widget)

Please notice that select single will throw an exception now if it doesn't find the object, instead of returning None as it did before. So I think that line is not necessary, if the widget is not present, an exception will be raised and the test will fail.

The test reads nicely. I'm just wondering if this can be run faster as a QtTest. But I'm not sure how the indicators are implemented, if this involves several projects, autopilot is probably the best option.

Marking as needs fixing only for the assertIsNotNone.
Thank you!

review: Needs Fixing (code review)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-06:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1844/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Allan LeSage (allanlesage) wrote on 2013-12-11:

#

Leo's on holiday but I've implemented his suggested changes; Saviq, are the unstable tests holding us back? (Note that dednick is also on holidays.)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2013-12-12:

#

FWIW dednick is not on holidays anymore (or at least was in yesterday)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2013-12-16:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/1901/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Albert Astals Cid (aacid) wrote on 2014-01-09:

#

What's the status of this?

Is it supposed to be held only by failing in CI?

I tried to rebuild the last rev but CI doesn't know about the job anymore.

Allan maybe you can merge master and push to get a rebuild?

review: Needs Information

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-01-09:

#

This is held by the issues with the prerequisite: https://code.launchpad.net/~allanlesage/unity8/indicator-stubs/+merge/192059/comments/464049

Bug #1238417 needs to be fixed for this to continue :/

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-10:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2051/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-10:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2052/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-10:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2053/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-01-10:

#

ATM the above basically says that the job passed, otto has issues with running u8 tests.

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-01-13:

#

While it all looks ok codewise, I think the opening/closing of the indicators should be provided by a higher level component than the widget itself, such as an Indicators emulator object, which could provide an open_indicator(identifier) function.

Marking as approved; emulators can be expanded later.

review: Approve

Revision history for this message

Nick Dedekind (nick-dedekind) wrote on 2014-01-13:

#

Just waiting on test suite run before global approve :)

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-01-13:

#

Apologies for the delay; merge of master forthcoming. . . .

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-14:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2060/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-01-14:

#

The new tests are failing; while running the new tests on my mako I'm seeing these failures as authentic: there's a new "shuffling" behavior which may be confusing the simple "does the header string match" test. Nick would you run these tests and give your opinion?

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-01-15:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2065/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-01-16:

#

Correction: an objectName for the IndicatorPage on which I was depending has been eliminated, need to discuss with dednick about how to get hold of that page again, marking this as "in progress" for the moment.

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-03-26:

#

Dusting off this ancient MP, some minor changes from its prior incarnation. Recall that this was blocked on getting 'indicator stubs' through, now we'd like to land to enable some indicator-network autopilot tests for Wellark.

Revision history for this message

Leo Arias (elopio) wrote on 2014-03-27:

#

This now got to 2014, so please add the new year.

57 + # TODO: submit to autopilot.introspection.types.Rectangle [alesage 2013-12-06]

Can you please report it as a bug in autopilot and put the link here on this comment?
That way we will know when we can remove the TODO.

65 + def swipe_to_open_indicator(self, window):
76 + def swipe_to_close_indicator(self, window):

These two are good places to put the autopilot logging decorator.
I'm not sure if you have used it before. If not, take a look at dash.py or ping me.

Also, as the class name is DefaultIndicatorWidget, maybe we can make the names shorter:
swipe_to_open
swipe_to_close

I leave that decision to you, if you think it's less clear, just leave them as they are now.

74 + # TODO: assert that the indicator page opened [alesage 2013-12-06]
85 + # TODO: assert that the indicator page closed [alesage 2013-12-06]

I think this is pretty important for the stability of the test. Are we missing something to be able to add a wait_for here?

143 + if platform.model() == "Desktop":
144 + self.skipTest("Test cannot be run on the desktop.")

Not a big deal, but after some discussion QAs seem to like more the single quotes, so I'm trying to convince people of this everywhere. Please change the double quotes for single.

206 + unity_proxy = self.launch_unity()
207 + unlock_unity(unity_proxy)

I would put these two statements on the setUp.

208 + widget = self.main_window.get_indicator_widget(self.indicator_name)
209 + widget.swipe_to_open_indicator(self.main_window)
210 + indicator_page = self.main_window.get_indicator_page(
211 + indicator_title=self.title
212 + )
213 + self.assertTrue(indicator_page.visible)

We could make this more page-object compliant changing the signatures of the methods.
get_indicator_widget and get_indicator_page are returning autopilot objects. That's the main clue that tells you they should be private and never accessed by a test.
swipe_to_open_indicator doesn't need you to pass the main_window as a parameter, because you can select it again doing something like self.get_root_instance().select_single(MainWindow)

So I would try to rewrite the test to something like:

indicator_page = self.main_window.open_indicator(self.indicator_name)
self.assertTrue(indicator_page.visible)
self.assertEqual(indicator_page.title, self.title)

That's of course pseudocode and I'm not sure if it's doable at all. Let me know if you need a hand and I'll dig into the vis, or we can pair program some time.

ps: Feel free to disagree with me on any of the things I've written. If you think that the current way it's better for some reason, I'm always glad to discuss.

This now got to 2014, so please add the new year.

57	+ # TODO: submit to autopilot.introspection.types.Rectangle [alesage 2013-12-06]

Can you please report it as a bug in autopilot and put the link here on this comment?
That way we will know when we can remove the TODO.

65	+ def swipe_to_open_indicator(self, window):
76	+ def swipe_to_close_indicator(self, window):

These two are good places to put the autopilot logging decorator.
I'm not sure if you have used it before. If not, take a look at dash.py or ping me.

Also, as the class name is DefaultIndicatorWidget, maybe we can make the names shorter:
swipe_to_open
swipe_to_close

I leave that decision to you, if you think it's less clear, just leave them as they are now.

74	+ # TODO: assert that the indicator page opened [alesage 2013-12-06]
85	+ # TODO: assert that the indicator page closed [alesage 2013-12-06]

I think this is pretty important for the stability of the test. Are we missing something to be able to add a wait_for here?

143	+ if platform.model() == "Desktop":
144	+ self.skipTest("Test cannot be run on the desktop.")

Not a big deal, but after some discussion QAs seem to like more the single quotes, so I'm trying to convince people of this everywhere. Please change the double quotes for single.

206	+ unity_proxy = self.launch_unity()
207	+ unlock_unity(unity_proxy)

I would put these two statements on the setUp.

208	+ widget = self.main_window.get_indicator_widget(self.indicator_name)
209	+ widget.swipe_to_open_indicator(self.main_window)
210	+ indicator_page = self.main_window.get_indicator_page(
211	+ indicator_title=self.title
212	+ )
213	+ self.assertTrue(indicator_page.visible)

We could make this more page-object compliant changing the signatures of the methods.
get_indicator_widget and get_indicator_page are returning autopilot objects. That's the main clue that tells you they should be private and never accessed by a test.
swipe_to_open_indicator doesn't need you to pass the main_window as a parameter, because you can select it again doing something like self.get_root_instance().select_single(MainWindow)

So I would try to rewrite the test to something like:

indicator_page = self.main_window.open_indicator(self.indicator_name)
self.assertTrue(indicator_page.visible)
self.assertEqual(indicator_page.title, self.title)

That's of course pseudocode and I'm not sure if it's doable at all. Let me know if you need a hand and I'll dig into the vis, or we can pair program some time.

ps: Feel free to disagree with me on any of the things I've written. If you think that the current way it's better for some reason, I'm always glad to discuss.

review: Needs Fixing (code review)

lp://staging/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget updated on 2014-03-27

518. By Allan LeSage on 2014-03-27: Remove emulator, preferring main_window opening of indicator.

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-03-27:

#

The need for a get_center has been served in the interim, thanks veebers for directing me to. I've adapted elopio's suggested style changes, eliminating the widget emulator--really the 'swipe' action belongs to an object that spans a few UI elements, e.g. the Tab, Widget, Page, and I'm not sure this fits the autopilot regime. But it's ok to just have that 'indicator' in the main_view for the moment IMO.

I'm blocked on getting the feedback for 'is the page open', will ask dednick about in the AM.

lp://staging/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget updated on 2014-03-31

519. By Allan LeSage on 2014-03-28: Restore platform import (cursing pyflakes), sphinxify a return in main_window emulator.
520. By Allan LeSage on 2014-03-31: Autopilot wait for indicator to be fullyOpened.

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-03-31:

#

Ok the property was suitably named 'fullyOpened', elopio if you could offer a final review please.

lp://staging/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget updated on 2014-04-01

521. By Allan LeSage on 2014-03-31: Year amendment for indicators autopilot tests.
522. By Allan LeSage on 2014-04-01: Removing an unused/tested 'close_indicator_page' function.
523. By Allan LeSage on 2014-04-01: Privatize get_indicator_widget and get_indicator_page.
524. By Allan LeSage on 2014-04-01: Adjust input import.

Revision history for this message

Leo Arias (elopio) on 2014-04-01:

#

review: Approve (code review)

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-04-02:

#

* Are there any related MPs required for this MP to build/function as expected? Please list.

No.

* Did you perform an exploratory manual test run of your code change and any related functionality?

This concerns adding an autopilot test and not new functionality; I've tested on mako and using the desktop emulator.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?

Packaging has not been changed.

* If you changed the UI, has there been a design review?

UI has not been changed.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-08:

#

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2756/rebuild

review: Approve (continuous-integration)

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-09:

#

Could we get a skip for Nexus10 here? It doesn't have bluetooth...

review: Needs Information

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-09:

#

Basically something like what I did in https://code.launchpad.net/~saviq/unity8/skip-bluetooth-on-manta/+merge/211834 - but better? :)

lp://staging/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget updated on 2014-04-09

525. By Allan LeSage on 2014-04-09: Merged trunk.
526. By Allan LeSage on 2014-04-09: Skip indicator-bluetooth on manta.
527. By Allan LeSage on 2014-04-09: Correct super invocation oops.

Revision history for this message

Allan LeSage (allanlesage) wrote on 2014-04-09:

#

Made that change, tested on manta, appears to work as expected :) .

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-10:

#

+1

* Did you perform an exploratory manual test run of the code change and any related functionality?
Yes.

* Did CI run pass? If not, please explain why.
Yup.

review: Approve

Revision history for this message

Michał Sawicz (saviq) wrote on 2014-04-10:

#

One thing to remember, this is locale-dependent, maybe we should export LANG=C somewhere?

Unity 8

Merge lp://staging/~allanlesage/unity8/autopilot-indicator-page-title-matches-widget into lp://staging/unity8

Commit message

Description of the change

Preview Diff

Subscribers