Bug 71816 – [Qt] run-qtwebkit-tests doesn't consider timeouts as failures

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Caio Marcelo de Oliveira Filho]

When the tst_* programs crash, run-qtwebkit-tests should account them as failures.

[János Badics]

Created attachment 116726 [details]
proposed patch

[Eric Seidel (no email)]

Comment on attachment 116726 [details]
proposed patch

Seems reasonable to me.  rs=me.

[Eric Seidel (no email)]

Comment on attachment 116726 [details]
proposed patch

Actually, wait.  We can unittest the master.cfg now.  Please add a unittest! :)

[János Badics]

Created attachment 122245 [details]
proposed patch

added unittest to the master.cfg

[WebKit Review Bot]

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.

[János Badics]

Created attachment 122409 [details]
proposed patch

corrected the design mistakes in the unittest for the master.cfg

[Csaba Osztrogonác]

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.

[János Badics]

Created attachment 124536 [details]
proposed patch

fixed the use of unnecessary variable 'logText'

[Csaba Osztrogonác]

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.

[János Badics]

Created attachment 126500 [details]
proposed patch

The unittest now tests the result of getText2 method too.

[Csaba Osztrogonác]

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.

[János Badics]

Created attachment 126746 [details]
proposed patch

Unified the assertions into one function

[Csaba Osztrogonác]

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.

[János Badics]

Created attachment 127819 [details]
proposed patch

Changed the precedence of Timeout FAILURE and WARNINGS and made some design changes

[Csaba Osztrogonác]

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".

[Csaba Osztrogonác]

Comment on attachment 127819 [details]
proposed patch

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