Code review comment for lp:~tpeeters/ubuntu-ui-toolkit/ap-toolbar-open

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

42 + return self.get_toolbar().close()

the close method doesn't return anything, so you can remove the 'return' from there.

50 + def open(self):
72 + def close(self):

Now we are missing tests for those two new public methods. It would be basically to copy the existing tests, but calling get_toolbar(). With the existing tests for the open and close methods of the main view, you can leave them as they are, or you could use a mock just to check that the new open and close methods were called.

95 + # ensure the toolbar is open
96 + self.open()

I find it weird that the click_button method opens the toolbar if it's not open. I think I would prefer to raise an exception if when called click_button the toolbar is not opened.

98 + # ensure the toolbar is still open (may have closed due to timeout)
99 + self.open()

I do like this other open. I think it's just how a real use would act.

119 +# def test_click_unexisting_button(self):

This test is failing for you probably because you are not using autopilot 1.4.

With this branch, we would be missing also a test that checks that the button is clicked even if the toolbar was closed during the method call. I think you can reproduce that condition changing self.pointing_device.move_to_object(button) for a mock that just sleeps until the toolbar is closed.

Thank you for working on this!

« Back to merge proposal