Code review comment for lp:~allanlesage/unity8/autopilot-indicator-page-title-matches-widget

9 +# Copyright (C) 2013 Canonical
32 +# Copyright (C) 2013 Canonical
94 +# Copyright (C) 2013 Canonical

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.