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

[Martin Peres]

On 27/06/17 20:44, Martin Peres wrote:
> 
> 
> 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?

Ping, QA hit the same issue...

Should I write the fix or do you want to take care of it?

Thanks,
Martin