Code review comment for lp://staging/~gary/launchpad/bug875697

On Nov 3, 2011, at 8:48 PM, Ian Booth wrote:

> Review: Approve code
>
> Hi Gary

Hi Ian. Thank you very much for your review.

> This looks like a nice change. I'll take it as a given that when the code calls page = client.load_page(), the page.content can be parsed onto a dict containing keys testCase, testName, type.

Right, if there is an incremental timeout. You can see the data that is used for this generated in the JS changes in this branch (handle_pass and handle_fail).

> If any one of those keys are missing, then:
>
> 76 + try:
> 77 + msg += (' The last test that ran to '
> 78 + 'completion before timing out was '
> 79 + '%(testCase)s:%(testName)s. The test %(type)sed.'
> 80 + % self._last_test_info)
> 81 + except KeyError:
> 82 + pass
>
> will not append anything to msg. Do we need to allow for that and still append a useful message fragment? Or is the test report dict always guaranteed to have all the required keys?

A good question. I added a snippet to alert a reader of the problem, and give a repr of the received data.

> Should there be any tests for this stuff? I would like to see some but I'll leave it to you.

I didn't see a reasonable way of making an integration test of our integration test runner stuff. Perhaps I should have shaved some more yaks to make it possible to do so, but I already have other yak shaving in progress, and I was (IMO) following an existing precedent for this level of code.

Thanks again.

Gary