[Piglit] [PATCH 3/3] framework: Exit if a filter removes all tests from a profile

[Martin Peres]

On 27/06/17 19:31, Dylan Baker wrote:
> Quoting Martin Peres (2017-06-27 03:38:33)
>> On 22/06/17 22:12, Dylan Baker wrote:
>>> Quoting Petri Latvala (2017-06-21 01:37:21)
>>>> On 06/20/2017 08:59 PM, Dylan Baker wrote:
>>>>> Quoting Petri Latvala (2017-06-20 05:41:11)
>>>>>> On 04/13/2017 09:46 PM, Dylan Baker wrote:
>>>>>>> Quoting Brian Paul (2017-04-12 13:04:59)
>>>>>>>> On 04/12/2017 11:55 AM, Dylan Baker wrote:
>>>>>>>>> It doesn't makes sense to run if a user has removed all tests from a
>>>>>>>>> selected profile, and currently if all tests are removed, then an
>>>>>>>>> assertion will be hit in the backend that isn't extremely clear about
>>>>>>>>> what went wrong. This should be much easier to understand.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Dylan Baker <dylanx.c.baker at intel.com>
>>>>>>>>> ---
>>>>>>>>>       framework/profile.py | 7 +++++++
>>>>>>>>>       1 file changed, 7 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/framework/profile.py b/framework/profile.py
>>>>>>>>> index 4604367e1..ce0b24ce8 100644
>>>>>>>>> --- a/framework/profile.py
>>>>>>>>> +++ b/framework/profile.py
>>>>>>>>> @@ -389,6 +389,13 @@ def run(profiles, logger, backend, concurrency):
>>>>>>>>>           profiles = [(p, list(p.itertests())) for p in profiles]
>>>>>>>>>           log = LogManager(logger, sum(len(l) for _, l in profiles))
>>>>>>>>>
>>>>>>>>> +    # check that after the filters are run there are actually tests to run
>>>>>>>>> +    for p, l in profiles:
>>>>>>>>> +        if not l:
>>>>>>>>> +            raise exceptions.PiglitUserError(
>>>>>>>>> +                'After running filters there are no tests in '
>>>>>>>>> +                'profile "{}"'.format(p.name))
>>>>>> Bumped into an issue caused by this commit with IGT tests.
>>>>>>
>>>>>> When the last test of a run never finishes and you attempt to
>>>>>>
>>>>>>       piglit resume -n test-results-path
>>>>>>
>>>>>> this exception is raised instead of the expected result of finishing up
>>>>>> the run.
>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Petri Latvala
>>>>>>
>>>>> That is expected with the command line you've supplied.
>>>>>
>>>>> The -n/--no-retry option instructs piglit to not retry tests that started but
>>>>> didn't finish, if you get the same behavior without the -n option that is a bug.
>>>>>
>>>>> Dylan
>>>>
>>>>
>>>> What is the supported method then to loop resume until results.json.bz2
>>>> gets generated? Previous behaviour was that piglit reported that there's
>>>> nothing more to do and generated that.
>>>>
>>>>
>>>> -- 
>>>> Petri Latvala
>>>>
>>>
>>> I would use `piglit summary aggregate` to build the results.json.foo file.
>>
>> Hi Dylan,
>>
>> Petri is on vacation for some time, so let me provide a little more
>> background on why this patch is problematic for our use-case.
>>
>> For automated IGT testing, we have a testlist and reboot the machine as
>> soon as we get a certain error condition or if the test takes more than
>> a specified timeout, which reboots the machine immediately (watchdog).
>>
>> Once the machine has rebooted, we re-deploy the right kernel and resume
>> piglit testing, using the -n option. When the testing is over, piglit
>> generates the results.json.foo file and returns 0. At this point,
>> ezbench considers the execution as done and does its own things.
>>
>> However, since this patch got introduced, if the last test got
>> interrupted, then piglit will raise the PiglitUserError exception which
>> is hard to interpret from outside as a way to say that all the tests
>> have been run and that we should be generating the report. This also
>> introduces an ill-tested codepath, so Petri and I were wondering why not
>> make it less confusing for the user by just realizing that there are no
>> tests left to run, and just generate the report at this point.
>>
>> Any suggestion as to what we should be doing? Maybe we could only send
>> this error when no tests have been run already and there are no runs to
>> be run?
>>
>> Thanks,
>> Martin
> 
> Hi Martin,
> 
> I really don't think we should remove the exception, the majority of piglit
> users are not wrapping piglit in more infrastructure, I think at this point the
> Intel kernel and mesa teams are the only ones doing so, and it does report a
> real error.

Sure, but let's not make it harder for other companies to use piglit, 
especially since piglit is the prefered runner for IGT which is becoming 
less intel-specific (AMD and vc4).

> 
> I do think that it would work if `piglit resume` would essentially become
> `piglit summary aggregate` if there are no tests left to run, since tests have
> actually run and a result would be non-empty.

Yes, that is all we are asking and it would make wrapping into more 
infrastructure easier :)

Feel like cooking the patch or should I do it?

Cheers,
Martin