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

[Ilia Mirkin]

On Tue, Feb 3, 2015 at 2:12 PM, Dylan Baker <baker.dylan.c at gmail.com> wrote:
> [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.

OK. I won't push on this, it's a minor point in my mind.

>
>>
>> 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.

While I was all for not allowing multiple args in a single string, I
can't say I would have approved of the change that switched everything
to have [] all over the place if I had seen it when it went in. I
guess it's not a huge deal, just a bit jarring to read through. I
would imagine it's best to remove as much boilerplate as possible, and
taking a single arg + a bunch of kw args is basically the same as a
*args + a bunch of kw args. Previously there was no easy way to pass
multiple args other than sticking spaces in, now you no longer split
the string up (I assume) so that won't work and thus is unlikely to
reoccur.

>
> 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...)

OK, excellent. Just wanted to make sure it wasn't an oversight.

  -ilia