Code review comment for lp:~linaro-infrastructure/tcwg-web/cbuild-lava

Hi,

thanks for taking a look!

On Wed, Jan 16, 2013 at 4:55 AM, Michael Hudson-Doyle
<email address hidden> wrote:
>> +
>> +
>> +def get_log_dir_name(lava_log):
>> + """Returns the dir name found in the LAVA log file.
>> +
>> + :param lava_log: The name of the saved LAVA log file.
>> + :type str
>> + """
>> + # We need to match strings in the form of:
>> + # + lava-test-case-attach [...] results[...]
>> + matcher = re.compile("\s*\+?\s*lava-test-case-attach\s*.*\s*results/")
>> + path = None
>> + if lava_log:
>> + lines = None
>> + try:
>> + # XXX Probably not the most efficient way of parsing the file.
>> + # Might be better to read it backwards, since look like the strings
>> + # we need are closer to the bottom of the file.
>> + with open(lava_log, 'r') as log:
>> + lines = log.readlines()
>> + for line in lines:
>> + if matcher.match(line):
>> + match = line.strip().split(' ')[-1]
>> + break
>> + if match:
>> + # We treat it like a path.
>> + path = os.path.dirname(match).split('/')[-1]
>> + finally:
>> + os.unlink(lava_log)
>> + return path
>
> Wow, this seems SUPER fragile.

It is, indeed. At the moment that was the first thing, and actually
the only place, where we could find that information.

> Can't you create an attachment that has
> the name of this directory in it?

I honestly have no idea, for I have yet to find out in cbuild code
where that directory name is created.

> Test cases can have attributes too,
> which would be more appropriate somehow, but there is no api for
> lava-test-shell tests to create them yet. Adding one will be easier
> than working out what this function is doing :-)

It would indeed, but I guess first we need to find out where that
directory name comes out.

Some of the other comments have been addressed, some are in the process.
Thanks again!

--
Milo Casagrande | Infrastructure Team
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs