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

Revision history for this message
Leo Arias (elopio) wrote :

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)

« Back to merge proposal