Code review comment for lp:~autopilot/unity8/autopilot-test-refactoring

W dniu 23.07.2013 12:20, Michael Zanetti pisze:
> Review: Needs Fixing code
>
> 160 +def get_grid_size():
> 161 + grid_size = os.getenv('GRID_UNIT_PX')
> 162 + if grid_size is None:
> 163 + raise RuntimeError(
> 164 + "Environment variable GRID_UNIT_PX has not been set."
> 165 + )
> 166 + return int(grid_size)
>
> Grid units have a fallback of 8 so this isn't necessarily needed. OTOH, it might makes sense to have this check to make sure we're always testing with something we've set ourselves?

Yeah, on the one hand we should make sure we're running at a certain GU,
on the other our tests should pass regardless of GU... I'm not sold on
either ;)

> 235 + super(TestDisplayMenus, self).setUp("400x800", self.grid_unit)
>
> Humm... 400x800 and a variable grid unit size? that can have nasty side effects in certain circumstances, I'd assume.

Fixed.

> 451 + # TODO: Is this ever called? Find out, and maybe remove this branch:
>
> This doesn't really make sense to me... Would make the whole emulator file useless...

Yeah, looks spurious.

> 496 + # check if it's availble before even attempting.
>
> Typo... Also, is this TODO really needed? If someone calls this in a wrong situation the test will fail anyways and I guess the resulting error message is informative enough already.

Dropped.

> 519 + def dismiss(self):
> 520 + """Closes the open Hud."""
> 521 + # Ensure that the Hud is actually open
> 522 + self.shown.wait_for(True)
> 523 + touch = Touch.create()
> 524 + x, y = self.get_close_button_coords()
> 525 + touch.tap(x, y)
>
> Would it make sense to wait for hidden too?

Done.

> 1402 + # TODO: perhaps move this to the Hud emulator?
>
> Imho something that should either be done or not. But not worth a TODO that will never be fixed.
>
> Not sure if all those are related to this branch, but there are some pyflakes issues.
>
> indicators_client/emulators/__init__.py:12: 'sleep' imported but unused
> indicators_client/emulators/__init__.py:14: 'IntrospectableObjectMetaclass' imported but unused
> indicators_client/emulators/__init__.py:31: undefined name 'IndicatorsTestCase'
> indicators_client/tests/__init__.py:14: 'Eventually' imported but unused
> indicators_client/tests/__init__.py:16: 'Equals' imported but unused
> indicators_client/tests/__init__.py:21: 'sys' imported but unused
> indicators_client/tests/__init__.py:22: 'sleep' imported but unused
> indicators_client/tests/test_battery.py:15: 'sleep' imported but unused
> indicators_client/tests/test_battery.py:17: 'math' imported but unused

We'll get to those in a second run.
--
Michał (Saviq) Sawicz <email address hidden>
Canonical Services Ltd.