Code review comment for lp://staging/~wesley-wiedenmeier/curtin/partial-testing

Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

> Hi Wesley,
>
> I just ran your branch to take a look and found some minor things I'd ask you
> if you consider them useful to add.
>
> The json files - while being json and not intended for humans - would still be
> much more readable it they had some line breaks. Like what "python -m
> json.tool /tmp/foo/storage-events.json" gives me would be great.

Yeah, that makes sense. I'll switch it to use something like 'json.dump(... indent=4)'

> Then in the log a few outputs seem not to go through LOG.
> I have a few questions about some of them:
>
> 1. Parse storage tests reporting data and ensure that all tests passed ... ok
> Should get a "2016-07-12 07:15:09,184 - vmtests - INFO -" prefix like the
> others IMHO.

Oh, that's the docstring for the test function, I think that's just added to nosetests's output when running with '-vv'. It may be good to emit a message through LOG though and remove the docstring cause the times could be good for debugging.

> 2. 10.245.168.14 - - [12/Jul/2016 07:15:22] "GET /storagetest-reporting.json
> HTTP/1.1" 200 -
> 10.245.168.14 - - [12/Jul/2016 07:15:22] "GET /storagetest-disk-conf.json
> HTTP/1.1" 200 -
> 10.245.168.14 - - [12/Jul/2016 07:15:22] "GET /curtin.tar.xz HTTP/1.1" 200 -
>
> What are thos actually doing, it is always the same text - Could be something
> like:
> 2016-07-12 07:15:09,184 - vmtests - INFO - fetching json reporting
> 2016-07-12 07:15:09,184 - vmtests - INFO - fetching json disk config
> 2016-07-12 07:15:09,184 - vmtests - INFO - fetching foo tarball

I'm not sure if I can actually change that or not, those messages come from http.server, and I don't know if there is an interface in it to handle custom messages, I'll check in the source for it though.

> 3. just after "10.245.168.14 - - [12/Jul/2016 07:15:22] "GET /curtin.tar.xz
> HTTP/1.1" 200 -" is the longest "wait" duration in the tests. There should be
> some sort of "doing the test now" or anything else that somebody looking at it
> knows what is taking the time. This doesn't have to be step 1,2,3,4,../20 -
> just after the "Booting target image" a short "running test XY now".

Yeah, it would definitely be nice to have a bit of debug there. I can add in optional logging to tools.report_webhook_logger to handle outputting events to stdio as well as the logfile when they're received, that should work okay

« Back to merge proposal