Bug 170523 – Add support for test262 JavaScriptCore tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

OpenSource bots should run test262 JSC tests. These are useful tests and we should have automated coverage for these.

See <rdar://problem/27673736>.

[Aakash Jain]

Created attachment 306339 [details]
Proposed patch

[Build Bot]

Attachment 306339 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:146:  [RunTest262TestsTest.assertResults] Undefined variable 'RunTest262Tests'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:155:  [RunTest262TestsTest.test_no_regressions_output] Undefined variable 'SUCCESS'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:159:  [RunTest262TestsTest.test_failure_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/mastercfg_unittest.py:166:  [RunTest262TestsTest.test_failures_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 306339 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306339&action=review

Looks sane to me.

> Tools/ChangeLog:12
> +        (RunTest262Tests.countFailures): method ot count the failures.

method => Method
ot => to

> Tools/ChangeLog:13
> +        (Test262Factory): Added Test262 Factory class.

Factory => factory

> Tools/ChangeLog:16
> +        (RunTest262TestsTest.assertResults): helper method.

helper => Helper

> Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:337
> +        logText = cmd.logs['stdio'].getText()

' => "

[Joseph Pecoraro]

There are a series of patches that gets test262 expectation failures down to 0. All the changes are in commit-queue now so should be landed by tomorrow so this bot can start out green.

[Keith Miller]

Comment on attachment 306339 [details]
Proposed patch

Awesome, this is very cool. Thanks for getting everything working Aakash and Joe!

[Aakash Jain]

Created attachment 306458 [details]
Updated patch

Changed from Sierra to El Capitan since the available bots have el capitan.

Fixed typos in ChangeLog.

[Joseph Pecoraro]

(In reply to Aakash Jain from comment #6)
> Created attachment 306458 [details]
> Updated patch
> 
> Changed from Sierra to El Capitan since the available bots have el 

Why not just update the bots to Sierra? Don't we control the bots?

[Alexey Proskuryakov]

> Why not just update the bots to Sierra? Don't we control the bots?

These are Xserves, which are not supported in macOS Sierra.

[Aakash Jain]

Created attachment 306523 [details]
Updated patch

Added "Reviewed by".

[WebKit Commit Bot]

Comment on attachment 306523 [details]
Updated patch

Clearing flags on attachment: 306523

Committed r215108: <http://trac.webkit.org/changeset/215108>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Aakash Jain]

working fine: https://build.webkit.org/dashboard/

[Carlos Alberto Lopez Perez]

I'm thinking in adding this step also for the GTK+ bots so they also run this test.

However.. I'm wondering why you added new bots that only run this tests instead of making the current bots that run JSC tests run this by simply adding the test path to this list https://trac.webkit.org/browser/trunk/Tools/Scripts/run-javascriptcore-tests?rev=215108#L268 ???
Its because of the time it takes to run to not slow down the current bots, or there is any other reason?


Due to this I have also checked that there are other two tests that the bots are not currently running: JSTests/heapProfiler.yaml and JSTests/asyncFunctionTests.yaml 

Is there any reason to not run this two tests on the bots?

[Joseph Pecoraro]

(In reply to Carlos Alberto Lopez Perez from comment #13)
> I'm thinking in adding this step also for the GTK+ bots so they also run
> this test.
> 
> However.. I'm wondering why you added new bots that only run this tests
> instead of making the current bots that run JSC tests run this by simply
> adding the test path to this list
> https://trac.webkit.org/browser/trunk/Tools/Scripts/run-javascriptcore-
> tests?rev=215108#L268 ???
> Its because of the time it takes to run to not slow down the current bots,
> or there is any other reason?

I actually don't know if there was a good reason for it. I just know that there were 2 machines available, so we used them for this. So its kind of a bonus that it doesn't make any of the other bots slow down. These tests are rather quick though, running in 9-10 minutes on the bots per commit.


> Due to this I have also checked that there are other two tests that the bots
> are not currently running: JSTests/heapProfiler.yaml and
> JSTests/asyncFunctionTests.yaml 
> 
> Is there any reason to not run this two tests on the bots?

We should probably add these to the normal JavaScriptCore tests bot. It seems like an oversight. Please file a new bugzilla bug. (CC me and Aakash!)

[Alexey Proskuryakov]

Not making regular tests slower was the reason for using a separate queue indeed.

[Caitlin Potter (:caitp)]

(In reply to Carlos Alberto Lopez Perez from comment #13)
> I'm thinking in adding this step also for the GTK+ bots so they also run
> this test.
> 
> However.. I'm wondering why you added new bots that only run this tests
> instead of making the current bots that run JSC tests run this by simply
> adding the test path to this list
> https://trac.webkit.org/browser/trunk/Tools/Scripts/run-javascriptcore-
> tests?rev=215108#L268 ???
> Its because of the time it takes to run to not slow down the current bots,
> or there is any other reason?
> 
> 
> Due to this I have also checked that there are other two tests that the bots
> are not currently running: JSTests/heapProfiler.yaml and
> JSTests/asyncFunctionTests.yaml 
> 
> Is there any reason to not run this two tests on the bots?

asyncFunctionTests.yaml was only there for convenience before shipping. Those should be covered by the stress test suite now, and IIRC they are. I will send a patch to clean it up tomorrow if nobody else does.

[Carlos Alberto Lopez Perez]

(In reply to Joseph Pecoraro from comment #14)
> (In reply to Carlos Alberto Lopez Perez from comment #13)
> > I'm thinking in adding this step also for the GTK+ bots so they also run
> > this test.
> > 
> > However.. I'm wondering why you added new bots that only run this tests
> > instead of making the current bots that run JSC tests run this by simply
> > adding the test path to this list
> > https://trac.webkit.org/browser/trunk/Tools/Scripts/run-javascriptcore-
> > tests?rev=215108#L268 ???
> > Its because of the time it takes to run to not slow down the current bots,
> > or there is any other reason?
> 
> I actually don't know if there was a good reason for it. I just know that
> there were 2 machines available, so we used them for this. 

I think you can add more workers to any slave. So you can have N machines for the JSC tester.

Check for example the Capitan Release WK2 tester: https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29

 ^^ It has 2 buildslaves connected (at the right-top you can see: bot208 and bot209)


> So its kind of a
> bonus that it doesn't make any of the other bots slow down. These tests are
> rather quick though, running in 9-10 minutes on the bots per commit.
> 
> 

But this complicates things from an infrastructure point of view. Adding a new test to a current set of tests makes it automatically work on all platforms and bots. Adding a new type of bot requires new hardware and new deployments. It also has to be done by each one of the port/bot admins.

In the GTK+ bot case it will be much easier for me to make the current bots run this extra step rather than adding new bots. 

However, if you still want to keep this test262 bots, then I guess I can do this change only for the GTK+/JSCOnly ports: that is to add the test262 step for the JSC bot only if platform is GTK+ or JSCOnly.

On the other hand, if you think it will be a good idea to merge test262 inside run-javascript-tests then things can be simplified quite a lot.

> > Due to this I have also checked that there are other two tests that the bots
> > are not currently running: JSTests/heapProfiler.yaml and
> > JSTests/asyncFunctionTests.yaml 
> > 
> > Is there any reason to not run this two tests on the bots?
> 
> We should probably add these to the normal JavaScriptCore tests bot. It
> seems like an oversight. Please file a new bugzilla bug. (CC me and Aakash!)

Filled: https://bugs.webkit.org/show_bug.cgi?id=171578