Bug 140821 – [buildbot] Simplify jscore-test buildstep

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

Created attachment 245222 [details]
Patch

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

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.

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

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

[Alexey Proskuryakov]

> 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!

[Csaba Osztrogonác]

Created attachment 245341 [details]
Patch

It depends on bug140884, which fixes the grammar issue of TestWithFailureCount.

[WebKit Commit Bot]

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.

[Csaba Osztrogonác]

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.

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

ping?

[Alexey Proskuryakov]

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.

[Csaba Osztrogonác]

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

[Csaba Osztrogonác]

(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"
        ]
      ], 

...

[Alexey Proskuryakov]

Comment on attachment 245341 [details]
Patch

I see.

[WebKit Commit Bot]

Comment on attachment 245341 [details]
Patch

Clearing flags on attachment: 245341

Committed r179342: <http://trac.webkit.org/changeset/179342>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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.

[Csaba Osztrogonác]

It works again.