[webkit-reviews] review denied: [Bug 71816] [Qt] run-qtwebkit-tests doesn't consider crashes as failures : [Attachment 124536] 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 crashes as failures
https://bugs.webkit.org/show_bug.cgi?id=71816

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

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


r- now due to inline comments. And we should add asserts to test the return 
value of getText2() too, not only the SUCCESS/WARNINGS/FAILURE result.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:582
> +	   foundItems = re.findall("TOTALS: (?P<passed>\d+) passed,
(?P<failed>\d+) failed, (?P<skipped>\d+) skipped", cmd.logs['stdio'].getText())

> +	   if foundItems:
> +	       self.incorrectTests = int(foundItems[0][1])
> +

As I said previously we shouldn't duplicate this code, because the 
number of failing test is calculated by commandComplete() function.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:586
> +	   if re.findall("Timeout, process", logText):

logtext doesn't exist here. Didn't you mean "cmd.logs['stdio'].getText()" ?

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:587
> +	       self.statusLine = ["Failure: timeout occured while testing\n%s
passed, %s failed, %s skipped" % (foundItems[0][0], foundItems[0][1],
foundItems[0][2])]

We should only append "Failure: timeout occured while testing" to statusLine.
Or overwriting it would be better, because if a timeout occured, results are
invalid.

And in this case self.incorrectTests is stronger than a timeout. It should be
reversed.

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:132
> +ERROR:Exec:Timeout, process
'WebKitBuild/Release/Source/WebKit2/UIProcess/API/qt/tests/qmltests/tst_qmltest
s' (2336) was terminated

This test case is incorrect. When we test only failures, 
there isn't timeout. But it would be better if you add one 
more testcase to test failures and timeout simultaneously too.