Bug 58370 – Move exit-after-n-failures count into commitqueuetask in preparation for increasing it

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Eric Seidel (no email)]

Move exit-after-n-failures count into commitqueuetask in preparation for increasing it

[Eric Seidel (no email)]

Created attachment 89261 [details]
Patch

[Adam Barth]

Comment on attachment 89261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89261&action=review

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:152
> +            " ".join(self._run_webkit_tests_args()),

That's too bad.  Can use we some sort of robust serialization / deserialization instead?  Maybe JSON?

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 89261 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=89261&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:152
> > +            " ".join(self._run_webkit_tests_args()),
> 
> That's too bad.  Can use we some sort of robust serialization / deserialization instead?  Maybe JSON?

I mean, we're passing through the shell here.  We could invent a way, sure.  But this seems like it fits the unix model.  Yes, this isnt' robust enough to handle all cases.

I'm kinda against being able to pass argumetns into run-webkit-tests at all, but it seems useful in the commit-queue case.  Unless we were to invent some sort of config-file system for running webkit-patch with a certain config.

[Adam Barth]

Maybe we should just pass the number of tests to run?  That's less general, but do we have examples of other arguments we want to pass?  Maybe shlex has some quoting functions we can use?

[Eric Seidel (no email)]

I originally wrote that part of the patch, to handle passing ignored tests.  (From the approach you didn't like.)  I'm not sure if we'll have other arguments.  And it's possible we dont' even need to do this. But it seemed odd for the commitqueuetask to know the behavior of runtests.py.

But I can also just chuck this patch.  After further though, it's not on the critical path.

[Ojan Vafai]

Comment on attachment 89261 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=89261&action=review

>>> Tools/Scripts/webkitpy/tool/bot/commitqueuetask.py:152
>>> +            " ".join(self._run_webkit_tests_args()),
>> 
>> That's too bad.  Can use we some sort of robust serialization / deserialization instead?  Maybe JSON?
> 
> I mean, we're passing through the shell here.  We could invent a way, sure.  But this seems like it fits the unix model.  Yes, this isnt' robust enough to handle all cases.
> 
> I'm kinda against being able to pass argumetns into run-webkit-tests at all, but it seems useful in the commit-queue case.  Unless we were to invent some sort of config-file system for running webkit-patch with a certain config.

I'm confused. Why serialize at all? can't you just create the list in  a local variable and extend it?

[Eric Seidel (no email)]

(In reply to comment #6)
> > I'm kinda against being able to pass argumetns into run-webkit-tests at all, but it seems useful in the commit-queue case.  Unless we were to invent some sort of config-file system for running webkit-patch with a certain config.
> 
> I'm confused. Why serialize at all? can't you just create the list in  a local variable and extend it?

Will that survive the trip through popen?  These are being passed to a separate process...  I don't think it will.

[Adam Barth]

Comment on attachment 89261 [details]
Patch

I believe that Eric took a different approach to this issue.  Please re-nominate if I'm mistaken.

[Eric Seidel (no email)]

The fixes are orthogonal.  But this one is not required for what I wanted to do.  So if it's viewed as more ugly and helpful, let's just close the bug.