RunJavaScriptCoreTests can be TestWithFailureCount as other tester steps. We would need only one countFailures instead of evaluateCommand, getText and getText2. actual.html log can be removed too, it is used by only the old style Mozilla JSC tests (run-javascriptcore-tests --no-jsc-stress). Now only Windows bots run this test, but this log isn't useful for anything, and Windows bots should switch to the modern JSC stress test to get better test coverage. And the empty logs on other bots are confusing.
Created attachment 245222 [details] Patch
Attachment 245222 [details] did not pass style-queue: ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125: [RunJavaScriptCoreTestsTest.test_mozilla_failure_old_output] Undefined variable 'FAILURE' [pylint/E0602] [5] ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:130: [RunJavaScriptCoreTestsTest.test_mozilla_failures_old_output] Undefined variable 'FAILURE' [pylint/E0602] [5] ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:135: [RunJavaScriptCoreTestsTest.test_jsc_stress_failure_new_output] Undefined variable 'FAILURE' [pylint/E0602] [5] ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:139: [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_new_output] Undefined variable 'FAILURE' [pylint/E0602] [5] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Just a note: I proposed to enable JSC stress tests on Windows by default in bug128307. Once that patch landed actual.html won't exist on any bot.
Comment on attachment 245222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245222&action=review > Tools/ChangeLog:3 > + [buildbot] Simplify jscore-test buildstep Longer term, we need to output a JSON file with results that build.webkit.org/dashboard could parse and display in a pop-up. Probably also split the failure output into separate files per test, as searching for the failures in a gigantic single log is a waste of time. This seems orthogonal to this patch though. Another orthogonal comment: we should probably consider not running JSC stress tests on regular bots all the time, they are super slow, and probably take away more value by delaying results than they add by running the tests. > Tools/ChangeLog:9 > + (RunJavaScriptCoreTests): Inherited from TestWithFailureCount and removed useless actual.html logfile. I tried to find any failing "Mozilla" tests in recent test runs, and couldn't. But isn't this file what is generated when Mozilla tests fail? > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:303 > + if match: > + return int(match.group(1)) This looks surprising. We don't add the failure counts if both Mozilla and JSC tests failed? Or does this get invoked twice for each test type? > Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125 > + self.assertResults(FAILURE, ["1 JSC tests failed"], 1, """Results for Mozilla tests: "1 tests" is not good grammar. Given that one failing test is the most common failure case, I think that the error text should be nice.
(In reply to comment #4) > Longer term, we need to output a JSON file with results that > build.webkit.org/dashboard could parse and display in a pop-up. Probably > also split the failure output into separate files per test, as searching for > the failures in a gigantic single log is a waste of time. JSC tests are almost always pass, regression happens very rarely and in this case the core JSC hackers fixes the issue immediately or roll out the patch and then fix offline. JSON results and exact output isn't really necessarry. > Another orthogonal comment: we should probably consider not running JSC > stress tests on regular bots all the time, they are super slow, and probably > take away more value by delaying results than they add by running the tests. On the release bots it takes only 3.5 minutes, but on the debug bots it takes near 30 minutes. > > Tools/ChangeLog:9 > > + (RunJavaScriptCoreTests): Inherited from TestWithFailureCount and removed useless actual.html logfile. > > I tried to find any failing "Mozilla" tests in recent test runs, and > couldn't. But isn't this file what is generated when Mozilla tests fail? It is, but only the Windows bot runs the Mozilla tests, other runs JSC stress test, which don't generate actual.html. > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:303 > > + if match: > > + return int(match.group(1)) > > This looks surprising. We don't add the failure counts if both Mozilla and > JSC tests failed? No, we can't run both of them in the same time, see http://trac.webkit.org/browser/trunk/Tools/Scripts/run-javascriptcore-tests?rev=175142#L204 > > Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125 > > + self.assertResults(FAILURE, ["1 JSC tests failed"], 1, """Results for Mozilla tests: > > "1 tests" is not good grammar. Given that one failing test is the most > common failure case, I think that the error text should be nice. I agree, it would be better to use the proper grammar here. The existing code do this, but the generic TestWithFailureCount can't handle it. To make it work, we should mark the position of the plural suffix in failedTestsFormatString somehow. It would fix the grammar of webkitpy, webkitperl, unit tests, ... too.
> JSC tests are almost always pass, regression happens very rarely They were a lot more flaky for several months this fall, and figuring out which ones failed most frequently was an important part of fixing the problem. Even now, we have random crashes on stress/poly-call-exit.js.ftl-eager. Looking at <https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29?numbuilds=200>, jsc failures are a very common occurrence, although this disproves my claim that single failure is the most common case. It used to be quite common recently though. > It is, but only the Windows bot runs the Mozilla tests, other runs JSC stress test I see!
Created attachment 245341 [details] Patch It depends on bug140884, which fixes the grammar issue of TestWithFailureCount.
Attachment 245341 [details] did not pass style-queue: ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125: [RunJavaScriptCoreTestsTest.test_mozilla_failure_old_output] Undefined variable 'FAILURE' [pylint/E0602] [5] ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:130: [RunJavaScriptCoreTestsTest.test_mozilla_failures_old_output] Undefined variable 'FAILURE' [pylint/E0602] [5] ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:135: [RunJavaScriptCoreTestsTest.test_jsc_stress_failure_new_output] Undefined variable 'FAILURE' [pylint/E0602] [5] ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:139: [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_new_output] Undefined variable 'FAILURE' [pylint/E0602] [5] Total errors found: 4 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
grammar fixed by http://trac.webkit.org/changeset/179116 and Windows bots already run stress tests since http://trac.webkit.org/changeset/179165 , so I think it's ready for landing now.
Comment on attachment 245341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245341&action=review > Tools/ChangeLog:4 > + [buildbot] Simplify jscore-test buildstep > + https://bugs.webkit.org/show_bug.cgi?id=140821 How does this change Buildbot's JSON results, will that break the dashboard? I'm not sure if I can tell from the patch.
(In reply to comment #10) > Comment on attachment 245341 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245341&action=review > > > Tools/ChangeLog:4 > > + [buildbot] Simplify jscore-test buildstep > > + https://bugs.webkit.org/show_bug.cgi?id=140821 > > How does this change Buildbot's JSON results, will that break the dashboard? > I'm not sure if I can tell from the patch. The JSON result would be changed a little bit, but it shouldn't affect the dashboard. eg. missing links for the empty actual.html and actual.html (source) log files, a little bit different result summary. I checked the dashboard code, it finds steps by name, which isn't changed, it is still "jscore-test". And it counts failures with "fail" word, which is still present in the output. I'm 99% sure it won't break the dashboard.
ping?
Comment on attachment 245341 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245341&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125 > - self.assertResults(FAILURE, ["jscore-test", '1 failing Mozilla test '], 1, """Results for Mozilla tests: > + self.assertResults(FAILURE, ["1 JSC test failed"], 1, """Results for Mozilla tests: The dashboard looks at testStep.results[1], and this patch moves output to results[0] AFAICT. It seems to become different from all other steps. One complication here is that the dashboard needs to work with both internal and open source buildbots, so changes to format need to be made synchronously.
(In reply to comment #13) > Comment on attachment 245341 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245341&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125 > > - self.assertResults(FAILURE, ["jscore-test", '1 failing Mozilla test '], 1, """Results for Mozilla tests: > > + self.assertResults(FAILURE, ["1 JSC test failed"], 1, """Results for Mozilla tests: > > The dashboard looks at testStep.results[1], and this patch moves output to > results[0] AFAICT. It seems to become different from all other steps. > > One complication here is that the dashboard needs to work with both internal > and open source buildbots, so changes to format need to be made > synchronously. Strange, only jsc tests was different, layout/perl/python tests dumps everything to the first line. Let me check it in the details.
(In reply to comment #13) > Comment on attachment 245341 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245341&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:125 > > - self.assertResults(FAILURE, ["jscore-test", '1 failing Mozilla test '], 1, """Results for Mozilla tests: > > + self.assertResults(FAILURE, ["1 JSC test failed"], 1, """Results for Mozilla tests: > > The dashboard looks at testStep.results[1], and this patch moves output to > results[0] AFAICT. It seems to become different from all other steps. > One complication here is that the dashboard needs to work with both internal > and open source buildbots, so changes to format need to be made > synchronously. I checked it, results[0] is the status of the buildstep, SUCCESS, FAIL, ... and result[1] contains the text results line by line, so it won't break the dashbord anywhere. Here is a good example: https://build.webkit.org/json/builders/Apple%20Win%207%20Debug%20%28Tests%29/builds/64427?as_text=1 "name": "jscore-test", "results": [ 2, [ "jscore-test", "6 failing JSC stress tests " ] ], ... "name": "layout-test", "results": [ 2, [ "63 failures", "45 new passes", "3 flakes", "3 missing results" ] ], ...
Comment on attachment 245341 [details] Patch I see.
Comment on attachment 245341 [details] Patch Clearing flags on attachment: 245341 Committed r179342: <http://trac.webkit.org/changeset/179342>
All reviewed patches have been landed. Closing bug.
Buildbot is died after is landed: https://build.webkit.org/waterfall Service Temporarily Unavailable The server is temporarily unable to service your request due to maintenance downtime or capacity problems. Please try again later. Apache/2.2 Server at build.webkit.org Port 443 Lucas, could you check what happened with the master? I thought changes are pushed to it manually by you. Otherwise this change is good, I checked it locally, so I have no idea what happened with the master.
It works again.