[webkit-reviews] review denied: [Bug 71816] [Qt] run-qtwebkit-tests doesn't consider timeouts as failures : [Attachment 126746] proposed patch

[bugzilla-daemon at webkit.org]

Csaba Osztrogonac <ossy at webkit.org> has denied János Badics
<dicska at gmail.com>'s request for review:
Bug 71816: [Qt] run-qtwebkit-tests doesn't consider timeouts as failures
https://bugs.webkit.org/show_bug.cgi?id=71816

Attachment 126746: proposed patch
https://bugs.webkit.org/attachment.cgi?id=126746&action=review

------- Additional Comments from Csaba Osztrogonac <ossy at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126746&action=review


> Tools/ChangeLog:4
> +	   [Qt] run-qtwebkit-tests doesn't consider crashes as failures
> +	   https://bugs.webkit.org/show_bug.cgi?id=71816

We forgot to change the title of the bug, now I did it. Please fix it here too.


> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:585
>	   if self.incorrectTests:
>	       return WARNINGS
>  
> +	   if re.findall("Timeout, process", cmd.logs['stdio'].getText()):
> +	       self.statusLine = "Failure: timeout occured while testing"
> +	       return FAILURE
> +

Timeout should be stronger than a simple fail.
Simple fail is only WARNINGS, but timeout is FAILURE.

But in this case if fails and timeout occure in the same time, simple 
fail will be stronger. So please change the order of these cases.
(And fix the fail_and_timeout unittest too.)

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:82
> +    def assertResults(self, expected_result, expected_out, stdio):

expected_text and actual_text would be better name than expected_out and gt2res

(same logic as the RunUnitTestsTest function)

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:147
> +	   self.assertResults(WARNINGS, "73 passed, 3 failed, 1 skipped",
"""INFO:Exec:Finished
WebKitBuild/Release/Source/WebKit/qt/tests/benchmarks/painting/tst_painting

It should be FAILURE, see my comments above.