Bug 204090 – [EWS] Parse jsc_results.json for JSC tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

Parse jsc_results.json for JSC tests. These results (from multiple jsc test runs) will help in determining whether the failures are introduced by the patch, or are pre-existing.

[Aakash Jain]

Created attachment 383323 [details]
Patch

[Aakash Jain]

Sample runs:
https://ews-build.webkit-uat.org/#/builders/21/builds/408
https://ews-build.webkit-uat.org/#/builders/21/builds/406

[EWS Watchlist]

Comment on attachment 383323 [details]
Patch

Attachment 383323 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13241808

New failing tests:
imported/blink/fast/events/panScroll-crash.html

[EWS Watchlist]

Created attachment 383339 [details]
Archive of layout-test-results from ews213 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews213  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Aakash Jain]

Created attachment 383360 [details]
Patch

[Aakash Jain]

- Added more unit-tests. 

- TestReRunJavaScriptCoreTests now runs all the unit-tests from TestRunJavaScriptCoreTests (by changing self.prefix accordingly).

- Updated platform/fullPlatform for few JSC unit-tests, follow-up from https://trac.webkit.org/changeset/250102/webkit , also added new unit-test to cover jsc-only scenario.


More sample runs: 
https://ews-build.webkit-uat.org/#/builders/22/builds/240
https://ews-build.webkit-uat.org/#/builders/21/builds/409

Note that build-step to Analyze all these failures would be added separately (in subsequent patch).

[Jonathan Bedard]

Comment on attachment 383360 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/steps.py:989
> +            self.setProperty(self.prefix + 'binary_failures', self.binaryFailures)

I'd like Zhifei to take a look at this too, the output format of run-javascriptcore-tests is something that confuses me...

> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1033
> +                        command=['perl', 'Tools/Scripts/run-javascriptcore-tests', '--no-build', '--no-fail-fast', '--json-output={0}'.format(self.jsonFileName), '--release', '--remote-config-file=remote-machines.json', '--jsc-only'],

Why the change here?

[Aakash Jain]

Comment on attachment 383360 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1033
>> +                        command=['perl', 'Tools/Scripts/run-javascriptcore-tests', '--no-build', '--no-fail-fast', '--json-output={0}'.format(self.jsonFileName), '--release', '--remote-config-file=remote-machines.json', '--jsc-only'],
> 
> Why the change here?

We didn't really had coverage for 'jsc-only' platform in unit-tests earlier. This test-case now covers 'jsc-only' platform. I set the 'platform' property to 'jsc-only', and the code (as expected) passes '--jsc-only' parameter to run-javascriptcore-tests.

[Aakash Jain]

Separated unit-tests changes related to https://trac.webkit.org/r250102 in Bug 204123.

[Aakash Jain]

Created attachment 383390 [details]
Patch after rebasing on ToT

[Jonathan Bedard]

Comment on attachment 383390 [details]
Patch after rebasing on ToT

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

I think blocking this on sorting out the right thing to do with https://bugs.webkit.org/show_bug.cgi?id=204091 is a bad idea, especially since we already have infrastructure that relies on the json file you're using here.

I do think that we'll be changing this in the near future, though. Maybe a FIXME in commandComplete with a bug about standardizing how we think about JSC test failures is a good idea. I don't think the standardization is in scope of this problem or https://bugs.webkit.org/show_bug.cgi?id=204091, but it does need to happen.

> Tools/BuildSlaveSupport/ews-build/steps.py:983
> +            self.binaryFailures.append('testapi')

I don't feel strongly about this, but we could reduce some duplication here:

for test in ['masm', 'air', 'b3', 'dog', 'api']:
    if jsc_results.get('all{}TestsPassed'.format(test.capitalize())) == False:
        self.binaryFailures.append('test{}'.format(test))

[Aakash Jain]

Comment on attachment 383390 [details]
Patch after rebasing on ToT

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

>> Tools/BuildSlaveSupport/ews-build/steps.py:983
>> +            self.binaryFailures.append('testapi')
> 
> I don't feel strongly about this, but we could reduce some duplication here:
> 
> for test in ['masm', 'air', 'b3', 'dog', 'api']:
>     if jsc_results.get('all{}TestsPassed'.format(test.capitalize())) == False:
>         self.binaryFailures.append('test{}'.format(test))

unfortunately that doesn't work for 'DFG'. 'DFG' is all caps, while 'Api', 'Masm' and 'Air' have only first character capitalized. I will keep this part of the patch as is for now.

[WebKit Commit Bot]

Comment on attachment 383390 [details]
Patch after rebasing on ToT

Clearing flags on attachment: 383390

Committed r252430: <https://trac.webkit.org/changeset/252430>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57166574>