Code review comment for lp:~pwlars/lava-test/dashboard-setup

Revision history for this message
James Westby (james-w) wrote :

Hi,

I think that the dashboard module could do with a module-level
docstring, as the name doesn't make it entirely obvious what it
is for.

74 + self.dashboardconf.set(self.section,'user', user)

Missing space.

137 + """
138 + Operate on results of previous test runs stored locally
139 + """

I don't think that is correct?

149 - 'tests.test_results']
150 + 'tests.test_results',
151 + 'tests.test_dashboard']

If you sort these it reduces conflicts.

179 +from fixtures import TestCaseWithFixtures
180 +
181 +class DashboardTests(TestCaseWithFixtures):

Two lines please.

196 + self.assertEqual(passwd, conf.password)
197 +
198 +class DashboardOutputTests(TestCaseWithFixtures):

Again, two blank lines please.

I agree that it would be nice to have tests of the interactivity.

You could use expect, or just write a known string to the standard
input, which amounts to the same thing but less powerful. That
also requires execing the script though. We can leave this gap in
our test coverage for now I guess.

The last point is that zyga said yesterday that he wasn't sure
what authentication strategy he wanted to use. That means we
might have to change this later if we go in a different direction,
but this code is fine right now.

Thanks,

James

review: Approve

« Back to merge proposal