Summary: | [Qt] run-qtwebkit-tests doesn't consider timeouts as failures | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Caio Marcelo de Oliveira Filho <cmarcelo> | ||||||||||||||||
Component: | Tools / Tests | Assignee: | János Badics <jbadics> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | jbadics, ossy, webkit.review.bot, webkit-sed | ||||||||||||||||
Priority: | P2 | Keywords: | Qt, QtTriaged | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Caio Marcelo de Oliveira Filho
2011-11-08 07:51:12 PST
Created attachment 116726 [details]
proposed patch
Comment on attachment 116726 [details]
proposed patch
Seems reasonable to me. rs=me.
Comment on attachment 116726 [details]
proposed patch
Actually, wait. We can unittest the master.cfg now. Please add a unittest! :)
Created attachment 122245 [details]
proposed patch
added unittest to the master.cfg
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.
Created attachment 122409 [details]
proposed patch
corrected the design mistakes in the unittest for the master.cfg
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. Created attachment 124536 [details]
proposed patch
fixed the use of unnecessary variable 'logText'
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. Created attachment 126500 [details]
proposed patch
The unittest now tests the result of getText2 method too.
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.
Created attachment 126746 [details]
proposed patch
Unified the assertions into one function
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. Created attachment 127819 [details]
proposed patch
Changed the precedence of Timeout FAILURE and WARNINGS and made some design changes
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 on attachment 127819 [details] proposed patch Landed in http://trac.webkit.org/changeset/108246 |