Code review comment for lp:~pwlars/lava-test/demo-parse

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

29 + testdata = json.loads(file(testdatafile,'r').read())
30 + test = abrek.testdef.testloader(testdata['testname'])
31 + try:
32 + test.parse(self.args[0]

Why read all the data and then not pass it to parse?

51 +class StreamTestParser(abrek.testdef.AbrekTestParser):
52 + def parse(self):
53 + super(StreamTestParser, self).parse()
54 + self.appendtoall({'units':'MB/s'})

This class isn't used, is it necessary?

104 + the parse() method should be called while already in the results
105 + directory and assumes that a file for test output will exist called
106 + testoutput.log.

Why not take a file descriptor with the test results? It's more flexible,
and chdir() isn't a great idea in library code.

122 + appenall=

typo in append.

182 + t['result'] = fixupdict[t['result']]

Do you want to assume that fixupdict will contain a key for every
result that will be found?

155 + def append(self,testid,entry):

Please add spaces after the comments in argument lists.

127 + self.results = {'testlist':[]}

Why a dict containing a list, and not just a list?

Seeing as you plan to remove cmd_parse I don't mind it missing tests,
but perhaps we should consider tests for the test_definitions?

Thanks,

James

review: Needs Information

« Back to merge proposal