Code review comment for lp:~azzar1/unity/fix-979686

Revision history for this message
Andrea Azzarone (azzar1) wrote :

> Hi,
>
> The test works and passes for me on my machine. However, i'd appreciate it if
> you could change your test a little bit:
>
> First, please replace "should" in your docstring to "must".
> 56 + def setUp(self):
> 57 + super(CategoryHeaderTests, self).setUp()
> 58 + self.dash.reveal_file_lens()
> 59 + self.lens = self.dash.get_current_lens()
>
> These lines of code don't belong in setUp(). They're not setting up the test
> environment for the test - they're part of the test execution. I'd be happy if
> you wanted to change the reveal_foo_lens methods to return the revealed lens,
> so this is one line instead of two. I'd also be happy if you wanted to put
> these lines of code into a separate method and call it from the start of your
> test.
>
> Also, if this test fails, you will be leaving the dash open. Can you please
> add a :
>
> self.addCleanup(self.dash.ensure_hidden) right after you reveal the
> application lens please? If your test passes, this is a no-op, so it's quite
> safe.
>
> Finally, is there a way we can make this test leave the lens in the same state
> as it found it? Can we un-expand the category header as well? Maybe add a
> second assert, so we know that clicking in that spot will bot expand and
> collapse the category header? What do you think?
>
>
> Cheers,

Done. What do you think now?

« Back to merge proposal