Merge lp://staging/~brendan-donegan/checkbox/piglit_wrapper into lp://staging/checkbox

117 + log_path = os.environ.get('CHECKBOX_DATA','.') + '/piglit-results/' + self._nam

I'm bike-shedding here but that's something I'd use os.path.join() for
Also there's a missing space after the first comma

156 + type=open,

This is wrong, use argparse.FileType("wt")

150 + parser.add_argument("--test", "-t",
151 + action='append',
152 + help="The expression used to get the tests to run.")

I'm not 100% sure here but action="append" without default=() will give you _either_ a list or None which is annoying from coding point of view. You may want to check if a default value makes sense here. A simple sanity check is to ask yourself what happens when you run the test program without the -t argument.

102 +from subprocess import check_output, PIPE, STDOUT
103 +from argparse import ArgumentParser

You may want to sort those

126 + piglit_output = check_output(run_command,
127 + universal_newlines=True,
128 + stderr=STDOUT)
129 + # TODO: Capture stderr instead?
130 + for line in piglit_output.split('\n'):
131 + if ' :: ' in line:
132 + self._results[line.split(' :: ')[-1].strip()] = \
133 + line.split(' :: ')[-2].strip()

This got me confused, arguably I never ran piglit but where is it printing the interesting bits to? stdout or stderr? If both then this is probably fine (but kill the comment), otherwise capture only the relevant stream.

112 + def run(self, output=None):
113 + piglit_output = ''
114 + if output:
115 + piglit_output = output.read()
116 + else:
117 + log_path = os.environ.get('CHECKBOX_DATA','.') + '/piglit-results/' + self._name

This is strange, the output parameter smells like something you'd do to unit-test the code below that does parsing.
If that is the case it'd be nice to write some actual tests. If not I don't understand what it is for. In either case
it feels less dirty to split this into _run and _parse that are easily testable without any mocking or just use the excellent mocker built into python so that the special-case argument is no longer needed.

Thanks
ZK

Looks good, merging, thanks!

er, based on zyga's comment, unapproving :/

Thanks for the review!

> 117 + log_path = os.environ.get('CHECKBOX_DATA','.') +
> '/piglit-results/' + self._nam
>
> I'm bike-shedding here but that's something I'd use os.path.join() for
> Also there's a missing space after the first comma

You're quite right. Also the whole thing needed a good pep8 clean, so I did that and it should take care of the second point

>
> 156 + type=open,
>
> This is wrong, use argparse.FileType("wt")

Moot point, since I removed the output option as a result of another comment below.

>
> 150 + parser.add_argument("--test", "-t",
> 151 + action='append',
> 152 + help="The expression used to get the tests to
> run.")
>
> I'm not 100% sure here but action="append" without default=() will give you
> _either_ a list or None which is annoying from coding point of view. You may
> want to check if a default value makes sense here. A simple sanity check is to
> ask yourself what happens when you run the test program without the -t
> argument.

This is slightly complicated and a bit messy. Actually the script really requires two different types of 'argument'. One is the regular expression to use to find the tests to run, the other is the name to give to the test run when storing the results. If I make both of these arguments as opposed to options, argparse can't seperate the two. I therefore need to make them both 'required options', which I should have done in the first place, but feel uneasy about doing because it's a non-sequitor. These being required makes the other point moot.

>
> 102 +from subprocess import check_output, PIPE, STDOUT
> 103 +from argparse import ArgumentParser
>
> You may want to sort those

Yep, done

>
> 126 + piglit_output = check_output(run_command,
> 127 + universal_newlines=True,
> 128 + stderr=STDOUT)
> 129 + # TODO: Capture stderr instead?
> 130 + for line in piglit_output.split('\n'):
> 131 + if ' :: ' in line:
> 132 + self._results[line.split(' :: ')[-1].strip()] = \
> 133 + line.split(' :: ')[-2].strip()
>
> This got me confused, arguably I never ran piglit but where is it printing the
> interesting bits to? stdout or stderr? If both then this is probably fine (but
> kill the comment), otherwise capture only the relevant stream.

I forget the details, but it was printing stuff to both, and I don't want any piglit output leaking into the script.

>
>
> 112 + def run(self, output=None):
> 113 + piglit_output = ''
> 114 + if output:
> 115 + piglit_output = output.read()
> 116 + else:
> 117 + log_path = os.environ.get('CHECKBOX_DATA','.') +
> '/piglit-results/' + self._name
>
> This is strange, the output parameter smells like something you'd do to unit-
> test the code below that does parsing.
> If that is the case it'd be nice to write some actual tests. If not I don't
> understand what it is for. In either case
> it feels ...

157 + help="""A friendly name for this group of tests
158 + to use in reporting.""")

This is minor but I don't think it does what you wanted. Look at --help.

But otherwise it's okay and I'm not opposed to landing it as is. Feel free to run the script with help and polish the output if you really want to.

Thanks
ZK

Attempt to merge into lp:checkbox failed due to conflicts:

text conflict in debian/changelog

I merged this manually while fixing the changelog conflict. Thanks!