Bug 71816 - [Qt] run-qtwebkit-tests doesn't consider timeouts as failures
Summary: [Qt] run-qtwebkit-tests doesn't consider timeouts as failures
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: János Badics
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks:
 
Reported: 2011-11-08 07:51 PST by Caio Marcelo de Oliveira Filho
Modified: 2012-02-20 08:58 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (1.29 KB, patch)
2011-11-28 03:52 PST, János Badics
eric: review-
Details | Formatted Diff | Diff
proposed patch (6.04 KB, patch)
2012-01-12 08:08 PST, János Badics
no flags Details | Formatted Diff | Diff
proposed patch (6.03 KB, patch)
2012-01-13 04:29 PST, János Badics
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
proposed patch (6.02 KB, patch)
2012-01-30 05:48 PST, János Badics
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
proposed patch (12.00 KB, patch)
2012-02-10 04:48 PST, János Badics
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
proposed patch (7.31 KB, patch)
2012-02-13 03:30 PST, János Badics
ossy: review-
ossy: commit-queue-
Details | Formatted Diff | Diff
proposed patch (7.22 KB, patch)
2012-02-20 08:07 PST, János Badics
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Caio Marcelo de Oliveira Filho 2011-11-08 07:51:12 PST
When the tst_* programs crash, run-qtwebkit-tests should account them as failures.
Comment 1 János Badics 2011-11-28 03:52:33 PST
Created attachment 116726 [details]
proposed patch
Comment 2 Eric Seidel (no email) 2011-12-21 13:51:18 PST
Comment on attachment 116726 [details]
proposed patch

Seems reasonable to me.  rs=me.
Comment 3 Eric Seidel (no email) 2011-12-21 13:51:44 PST
Comment on attachment 116726 [details]
proposed patch

Actually, wait.  We can unittest the master.cfg now.  Please add a unittest! :)
Comment 4 János Badics 2012-01-12 08:08:47 PST
Created attachment 122245 [details]
proposed patch

added unittest to the master.cfg
Comment 5 WebKit Review Bot 2012-01-12 08:11:44 PST
Attachment 122245 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/BuildSlaveSupport/build.webkit.org-c..." exit_code: 1

Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:80:  expected 2 blank lines, found 1  [pep8/E302] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:82:  multiple statements on one line (semicolon)  [pep8/E702] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:82:  missing whitespace around operator  [pep8/E225] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:87:  trailing whitespace  [pep8/W291] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:89:  trailing whitespace  [pep8/W291] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:108:  trailing whitespace  [pep8/W291] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125:  trailing whitespace  [pep8/W291] [5]
Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:144:  trailing whitespace  [pep8/W291] [5]
Total errors found: 8 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 János Badics 2012-01-13 04:29:55 PST
Created attachment 122409 [details]
proposed patch

corrected the design mistakes in the unittest for the master.cfg
Comment 7 Csaba Osztrogonác 2012-01-24 06:17:17 PST
Comment on attachment 122409 [details]
proposed patch

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

r- now, see the comments. 

And please check your patch with git diff --color 
to ensure not use <TAB> characters for indentation.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:579
> +    	logText = cmd.logs['stdio'].getText()

We don't need this local variable. Why don't you pass "cmd.logs['stdio'].getText()" to re.fidall directly?

> 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", logText)
> +        if foundItems:
> +            self.incorrectTests = int(foundItems[0][1])

Why did you copy these lines from commandComplete function?
We don't need to duplicate them.
Comment 8 János Badics 2012-01-30 05:48:08 PST
Created attachment 124536 [details]
proposed patch

fixed the use of unnecessary variable 'logText'
Comment 9 Csaba Osztrogonác 2012-01-30 08:03:13 PST
Comment on attachment 124536 [details]
proposed patch

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_qmltests' (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.
Comment 10 János Badics 2012-02-10 04:48:30 PST
Created attachment 126500 [details]
proposed patch

The unittest now tests the result of getText2 method too.
Comment 11 Csaba Osztrogonác 2012-02-10 04:56:29 PST
Comment on attachment 126500 [details]
proposed patch

r-, because we shouldn't duplicate stdio. We can check the return 
value and the getText2 return value with one function call.
Comment 12 János Badics 2012-02-13 03:30:49 PST
Created attachment 126746 [details]
proposed patch

Unified the assertions into one function
Comment 13 Csaba Osztrogonác 2012-02-20 07:49:30 PST
Comment on attachment 126746 [details]
proposed patch

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.
Comment 14 János Badics 2012-02-20 08:07:09 PST
Created attachment 127819 [details]
proposed patch

Changed the precedence of Timeout FAILURE and WARNINGS and made some design changes
Comment 15 Csaba Osztrogonác 2012-02-20 08:25:15 PST
Comment on attachment 127819 [details]
proposed patch

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

LGTM, r=me with minor fixes. I'll fix them before landing.

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:580
> +            self.statusLine = "Failure: timeout occured while testing"

You missed to add "[" and "]".

> Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:147
> +        self.assertResults(FAILURE, "API tests", """INFO:Exec:Finished WebKitBuild/Release/Source/WebKit/qt/tests/benchmarks/painting/tst_painting

The expected result is "Failure: timeout occured while testing" instead of "API tests".
Comment 16 Csaba Osztrogonác 2012-02-20 08:58:06 PST
Comment on attachment 127819 [details]
proposed patch

Landed in http://trac.webkit.org/changeset/108246