[Piglit] [RFC v2 00/39] Rework profile modification (all.py)

[Dylan Baker]

[snip]

On Monday, February 02, 2015 21:40:25 Ilia Mirkin wrote:
> I'd still encourage you to allow run_concurrent to be specified by the
> context manager, and then letting individual tests override as needed.
> Not a _huge_ deal I suppose, but definitely my personal preference. In
> fact making it always explicitly specified in the context might be a
> nice improvement for readability.

You can, and we can add another patch to do that. You can pass any
keywords to the context manager (other than name, which adder uses for
the second keyword, but we could always change that to something
unique), and they will be passed to the test class. I just really wanted
to make the change to concurrent by default, because for where piglit is
at this point that is the sane default.

> 
> I also can't say I'm a huge fan of g(['asdf']). What's wrong with
> g('asdf')? Use the list when there are multiple args... or just use
> multiple positional args, i.e. g('asdf', 'arg1')?

IMO it's better not to do tricky things with the arguments for the
test, it's too easy to break tests in subtle ways (I broke a lot of
tests in subtle ways writing this series). We moved to requiring the
list (instead of allow lists or strings) because people were passing
tests with spaces in the string and breaking tests on some platforms
(cygwin I think), I think it's better to enforce the right behavior.
Allowing strings currently would also put more code in the adder
function.

Doing tricky things with the name of the test is fine with me, it's
really obvious that the name has changed.

> 
> Other than that, the final result looks quite reasonable. One final question --
> 
> with profile.group_manager(
>   PiglitGLTest,
>   grouptools.join('spec', 'arb_draw_buffers_blend')) as g:
> 
> But the current code reads
> 
> spec['ARB_draw_buffers_blend'] = arb_draw_buffers_blend
> 
> It looks like you've lower-cased things which will cause _huge_ churn
> wrt an old results file. Can you run a before-and-after comparison
> with your changes? That will make sure that (a) you got all (or most)
> the tests and (b) none of the names changed accidentally.

Patch 8 changes the behavior of storing tests to be always lowered. So
ARB_draw_buffers_blend == arb_draw_buffers_blend. It's a compromise for
OSX and Windows because their file systems have less than sane character
comparison behavior. The commit message for that patch is pretty
thorough on my reasoning.

You can run summary on the two versions, you will go through 2 results
version bumps though (I just couldn't come up with a good way to handle
this without doing that. Actually, we probably should have gone through
5 versions, but I made a mega patch instead...)

> 
> Cheers,
> 
>   -ilia

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/piglit/attachments/20150203/e5f2b927/attachment.sig>