Bug 140821

Summary: [buildbot] Simplify jscore-test buildstep
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, commit-queue, fpizlo, ggaren, lforschler, ossy, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 140884    
Bug Blocks: 141165    
Attachments:
Description Flags
Patch
none
Patch none

Description Csaba Osztrogonác 2015-01-23 04:46:11 PST
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.
Comment 1 Csaba Osztrogonác 2015-01-23 04:53:11 PST
Created attachment 245222 [details]
Patch
Comment 2 WebKit Commit Bot 2015-01-23 04:54:17 PST
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.
Comment 3 Csaba Osztrogonác 2015-01-23 05:23:23 PST
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 4 Alexey Proskuryakov 2015-01-23 10:00:17 PST
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.
Comment 5 Csaba Osztrogonác 2015-01-23 11:00:57 PST
(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.
Comment 6 Alexey Proskuryakov 2015-01-23 11:22:38 PST
> 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!
Comment 7 Csaba Osztrogonác 2015-01-26 04:36:40 PST
Created attachment 245341 [details]
Patch

It depends on bug140884, which fixes the grammar issue of TestWithFailureCount.
Comment 8 WebKit Commit Bot 2015-01-26 04:38:00 PST
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.
Comment 9 Csaba Osztrogonác 2015-01-27 03:52:44 PST
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 10 Alexey Proskuryakov 2015-01-27 09:49:03 PST
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.
Comment 11 Csaba Osztrogonác 2015-01-27 10:07:13 PST
(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.
Comment 12 Csaba Osztrogonác 2015-01-28 10:26:44 PST
ping?
Comment 13 Alexey Proskuryakov 2015-01-28 11:01:10 PST
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.
Comment 14 Csaba Osztrogonác 2015-01-28 11:20:27 PST
(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.
Comment 15 Csaba Osztrogonác 2015-01-28 11:28:41 PST
(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 16 Alexey Proskuryakov 2015-01-28 11:37:51 PST
Comment on attachment 245341 [details]
Patch

I see.
Comment 17 WebKit Commit Bot 2015-01-29 05:32:25 PST
Comment on attachment 245341 [details]
Patch

Clearing flags on attachment: 245341

Committed r179342: <http://trac.webkit.org/changeset/179342>
Comment 18 WebKit Commit Bot 2015-01-29 05:32:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 19 Csaba Osztrogonác 2015-01-29 05:58:45 PST
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.
Comment 20 Csaba Osztrogonác 2015-01-29 06:32:41 PST
It works again.