Bug 199489 - run-javascriptcore-tests won't report test results for testmasm, testair, testb3, testdfg and test api
Summary: run-javascriptcore-tests won't report test results for testmasm, testair, tes...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 199797
Blocks:
  Show dependency treegraph
 
Reported: 2019-07-03 19:55 PDT by Zhifei Fang
Modified: 2019-07-24 11:07 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.35 KB, patch)
2019-07-03 20:07 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (1.30 KB, patch)
2019-07-08 14:29 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2019-07-09 15:23 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (5.70 KB, patch)
2019-07-10 10:53 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (5.74 KB, patch)
2019-07-10 14:59 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (6.28 KB, patch)
2019-07-11 14:39 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (6.39 KB, patch)
2019-07-12 11:26 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (6.42 KB, patch)
2019-07-12 12:08 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2019-07-16 15:38 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (9.08 KB, patch)
2019-07-17 10:36 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (9.09 KB, patch)
2019-07-17 10:59 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (8.95 KB, patch)
2019-07-22 13:58 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (9.67 KB, patch)
2019-07-23 11:56 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff
Patch (9.93 KB, patch)
2019-07-23 17:22 PDT, Zhifei Fang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhifei Fang 2019-07-03 19:55:10 PDT
run-javascript-core-test won't report json results for testmasm, testair, testb3, testdfg and test api without fail fast
Comment 1 Zhifei Fang 2019-07-03 20:07:57 PDT
Created attachment 373451 [details]
Patch
Comment 2 Build Bot 2019-07-03 22:02:41 PDT
Comment on attachment 373451 [details]
Patch

Attachment 373451 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12657707

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests
Comment 3 Aakash Jain 2019-07-08 12:21:05 PDT
Comment on attachment 373451 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:-406
> -    if ($testResult && $failFast) {

Why is testResult being removed from here? Doesn't that mean that irrespective of failure or success, we will always exit the script. 
Since the runTest() method is called multiple times (once each for testmasm, testair, testb3, testdfg, testapi), only 'testmasm' would be run, and rest of the tests would never be run.

Please verify.
Comment 4 Zhifei Fang 2019-07-08 14:29:18 PDT
Created attachment 373665 [details]
Patch
Comment 5 Aakash Jain 2019-07-08 14:40:16 PDT
Comment on attachment 373665 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:404
> +        writeJsonDataIfApplicable();

Since the runTest() method is called multiple times, wouldn't the json results be overwritten after every function call, and only 'testapi' results be reported?

may also modify 'writeJsonDataIfApplicable' to append (>>) instead of overwritting(>), and delete any old data from the file before starting the tests.
Comment 6 Zhifei Fang 2019-07-08 15:01:11 PDT
(In reply to Aakash Jain from comment #5)
> Comment on attachment 373665 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373665&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:404
> > +        writeJsonDataIfApplicable();
> 
> Since the runTest() method is called multiple times, wouldn't the json
> results be overwritten after every function call, and only 'testapi' results
> be reported?
> 
> may also modify 'writeJsonDataIfApplicable' to append (>>) instead of
> overwritting(>), and delete any old data from the file before starting the
> tests.

I found I have modified the wrong place, it should write the json file at then end. Will provide another patch.
Comment 7 Zhifei Fang 2019-07-09 15:23:59 PDT
Created attachment 373779 [details]
Patch
Comment 8 Build Bot 2019-07-09 15:28:39 PDT
Attachment 373779 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Build Bot 2019-07-09 18:38:34 PDT
Comment on attachment 373779 [details]
Patch

Attachment 373779 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12703212

New failing tests:
mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
apiTests
Comment 10 Zhifei Fang 2019-07-10 10:53:01 PDT
Created attachment 373850 [details]
Patch
Comment 11 Build Bot 2019-07-10 10:55:41 PDT
Attachment 373850 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Aakash Jain 2019-07-10 11:33:09 PDT
Comment on attachment 373850 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:413
> +        reportTestFailures();

Do you want to run this method only in case of failFast?

I see that this method is called at the end as well, but that would probably report test failures only for last function call.

> Tools/Scripts/run-javascriptcore-tests:421
> +  if ($numJSCtestFailures) {

Do you want to exit early here if numJSCtestFailures is 0?
Comment 13 Zhifei Fang 2019-07-10 11:46:09 PDT
(In reply to Aakash Jain from comment #12)
> Comment on attachment 373850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373850&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:413
> > +        reportTestFailures();
> 
> Do you want to run this method only in case of failFast?
> 
> I see that this method is called at the end as well, but that would probably
> report test failures only for last function call.

I call it at the end because I will have some format like this:



** The following JSC stress test failures have been introduced:
	mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
	mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
	mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl

Results for JSC stress tests:
    3 failures found.


** The following JSC test binaries failures have been introduced:
	testmasm
	testair
	testb3
	testdfg
	testapi

Results for JSC test binaries:
    5 failures found.



We will put all the report together. This will be good for bot watcher to look at.

Also, if we have fail fast option, that means it should quit whenever a failures found, we should also print this summary, it will look like this:


testApi completed with rc=5
** The following JSC test binaries failures have been introduced:
	testapi

Results for JSC test binaries:
    1 failure found.


> 
> > Tools/Scripts/run-javascriptcore-tests:421
> > +  if ($numJSCtestFailures) {
> 
> Do you want to exit early here if numJSCtestFailures is 0?


No, if there are no test failures, it should print out the summary as well:

like this:

Results for JSC test binaries:
    0 failure found.
OK

This is also the same logic for the jsc stress tests report.
Comment 14 Aakash Jain 2019-07-10 13:00:39 PDT
Comment on attachment 373850 [details]
Patch

looks fine to me, but would prefer if jbedard can also have a look.
Comment 15 Jonathan Bedard 2019-07-10 13:08:11 PDT
Comment on attachment 373850 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:359
>              return int(match.group(1))

Should we be early returning here?
Comment 16 Jonathan Bedard 2019-07-10 13:08:35 PDT
Can we see an example of old output vs new output?
Comment 17 Zhifei Fang 2019-07-10 13:44:52 PDT
(In reply to Jonathan Bedard from comment #16)
> Can we see an example of old output vs new output?

Only add a new section

** The following JSC stress test failures have been introduced:
	mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-ftl-eager-no-cjit-validate-phases
	mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-llint
	mozilla-tests.yaml/js1_5/Array/regress-101964.js.mozilla-no-ftl

Results for JSC stress tests:
    3 failures found.


+** The following JSC test binaries failures have been introduced:
+	testmasm
+	testair
+	testb3
+	testdfg
+	testapi
+
+Results for JSC test binaries:
+    5 failures found.
Comment 18 Zhifei Fang 2019-07-10 13:49:30 PDT
(In reply to Jonathan Bedard from comment #15)
> Comment on attachment 373850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373850&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:359
> >              return int(match.group(1))
> 
> Should we be early returning here?

That's the existing logic, it will return the count for "Results for Mozilla tests:" or "Results for JSC stress test:" I guess "Results for Mozilla tests:" is an old format, I don't find any script output this.
Comment 19 Jonathan Bedard 2019-07-10 14:13:24 PDT
Comment on attachment 373850 [details]
Patch

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

>>> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:359
>>>              return int(match.group(1))
>> 
>> Should we be early returning here?
> 
> That's the existing logic, it will return the count for "Results for Mozilla tests:" or "Results for JSC stress test:" I guess "Results for Mozilla tests:" is an old format, I don't find any script output this.

It's just weird to me that we converted the other early return but not this one.
Comment 20 Zhifei Fang 2019-07-10 14:59:08 PDT
Created attachment 373866 [details]
Patch
Comment 21 Zhifei Fang 2019-07-10 14:59:35 PDT
(In reply to Jonathan Bedard from comment #19)
> Comment on attachment 373850 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373850&action=review
> 
> >>> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:359
> >>>              return int(match.group(1))
> >> 
> >> Should we be early returning here?
> > 
> > That's the existing logic, it will return the count for "Results for Mozilla tests:" or "Results for JSC stress test:" I guess "Results for Mozilla tests:" is an old format, I don't find any script output this.
> 
> It's just weird to me that we converted the other early return but not this
> one.

changed
Comment 22 Build Bot 2019-07-10 15:00:49 PDT
Attachment 373866 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Jonathan Bedard 2019-07-10 17:05:31 PDT
jsc-mips is failing.

Is this because we were ignoring such failures earlier? Is this an existing failure?
Comment 24 Zhifei Fang 2019-07-10 17:08:47 PDT
(In reply to Jonathan Bedard from comment #23)
> jsc-mips is failing.
> 
> Is this because we were ignoring such failures earlier? Is this an existing
> failure?

I don't think without restart, the buildbot part code will have any effects.
Comment 25 Zhifei Fang 2019-07-10 17:10:41 PDT
(In reply to Jonathan Bedard from comment #23)
> jsc-mips is failing.
> 
> Is this because we were ignoring such failures earlier? Is this an existing
> failure?

I was wrong here, because I change the return code, if any test binaries failed, it will return 1 which indicate the error.
Comment 26 Jonathan Bedard 2019-07-10 17:12:01 PDT
(In reply to Zhifei Fang from comment #25)
> (In reply to Jonathan Bedard from comment #23)
> > jsc-mips is failing.
> > 
> > Is this because we were ignoring such failures earlier? Is this an existing
> > failure?
> 
> I was wrong here, because I change the return code, if any test binaries
> failed, it will return 1 which indicate the error.

Previous patches didn't fail, though. So this is either a flakey test or a newly introduced failure.
Comment 27 Jonathan Bedard 2019-07-10 17:24:29 PDT
Comment on attachment 373866 [details]
Patch

Zhifei says that the problem jsc-mips has surfaced may be a real issue.
Comment 28 Zhifei Fang 2019-07-10 17:27:48 PDT
(In reply to Jonathan Bedard from comment #27)
> Comment on attachment 373866 [details]
> Patch
> 
> Zhifei says that the problem jsc-mips has surfaced may be a real issue.

Yes, we cannot testb3 testair testdfg etc for arch MIPS, those only support X64 and ARM64
Comment 29 Guillaume Emont 2019-07-11 09:45:50 PDT
(In reply to Jonathan Bedard from comment #26)

> 
> Previous patches didn't fail, though. So this is either a flakey test or a
> newly introduced failure.

I think it's neither: I just enabled testing on the jsc-mips EWS. Up until yesterday, it was only compiling and not running tests.
Comment 30 Guillaume Emont 2019-07-11 09:48:40 PDT
(In reply to Guillaume Emont from comment #29)
> (In reply to Jonathan Bedard from comment #26)
> 
> > 
> > Previous patches didn't fail, though. So this is either a flakey test or a
> > newly introduced failure.
> 
> I think it's neither: I just enabled testing on the jsc-mips EWS. Up until
> yesterday, it was only compiling and not running tests.

I also don't fully understand why the bubbles for arm and mips are currently green, since when clicking on them we see that they are actually failing.
Comment 31 Jonathan Bedard 2019-07-11 09:57:54 PDT
(In reply to Guillaume Emont from comment #30)
> (In reply to Guillaume Emont from comment #29)
> > (In reply to Jonathan Bedard from comment #26)
> > 
> > > 
> > > Previous patches didn't fail, though. So this is either a flakey test or a
> > > newly introduced failure.
> > 
> > I think it's neither: I just enabled testing on the jsc-mips EWS. Up until
> > yesterday, it was only compiling and not running tests.
> 
> I also don't fully understand why the bubbles for arm and mips are currently
> green, since when clicking on them we see that they are actually failing.

It looks like they passed on retry.

Old EWS doesn't actually show us the output of successful run.
Comment 32 Zhifei Fang 2019-07-11 14:39:21 PDT
Created attachment 373956 [details]
Patch
Comment 33 Build Bot 2019-07-11 14:40:55 PDT
Attachment 373956 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Zhifei Fang 2019-07-11 14:48:01 PDT
(In reply to Jonathan Bedard from comment #31)
> (In reply to Guillaume Emont from comment #30)
> > (In reply to Guillaume Emont from comment #29)
> > > (In reply to Jonathan Bedard from comment #26)
> > > 
> > > > 
> > > > Previous patches didn't fail, though. So this is either a flakey test or a
> > > > newly introduced failure.
> > > 
> > > I think it's neither: I just enabled testing on the jsc-mips EWS. Up until
> > > yesterday, it was only compiling and not running tests.
> > 
> > I also don't fully understand why the bubbles for arm and mips are currently
> > green, since when clicking on them we see that they are actually failing.
> 
> It looks like they passed on retry.
> 
> Old EWS doesn't actually show us the output of successful run.

Those test binaries cannot be run on remote device actually, for now, this script don't have this functionality. Even you provide --remote to this command, all the test binaries will be executed locally. I guess we should fix in other patch.

And here, my newest patch will disable those test binaries for mips. Even though we change it to run on remote device, mips won't support those as well.
Comment 35 Guillaume Emont 2019-07-12 03:30:23 PDT
(In reply to Zhifei Fang from comment #34)
> (In reply to Jonathan Bedard from comment #31)
> > (In reply to Guillaume Emont from comment #30)
> > > (In reply to Guillaume Emont from comment #29)
> > > > (In reply to Jonathan Bedard from comment #26)
> > > > 
> > > > > 
> > > > > Previous patches didn't fail, though. So this is either a flakey test or a
> > > > > newly introduced failure.
> > > > 
> > > > I think it's neither: I just enabled testing on the jsc-mips EWS. Up until
> > > > yesterday, it was only compiling and not running tests.
> > > 
> > > I also don't fully understand why the bubbles for arm and mips are currently
> > > green, since when clicking on them we see that they are actually failing.
> > 
> > It looks like they passed on retry.
> > 
> > Old EWS doesn't actually show us the output of successful run.
> 
> Those test binaries cannot be run on remote device actually, for now, this
> script don't have this functionality. Even you provide --remote to this
> command, all the test binaries will be executed locally. I guess we should
> fix in other patch.
> 
> And here, my newest patch will disable those test binaries for mips. Even
> though we change it to run on remote device, mips won't support those as
> well.

Should be disabled for armv7 too. Don't know if there are other EWSs running tests remotely.

BTW, there is a patch awaiting review in Bug 195404 that runs tests on remote devices. We have an issue with a testmasm failure on arm that I'm currently investigating though, so we might not want to enable this immediately.
Comment 36 Guillaume Emont 2019-07-12 03:35:30 PDT
Comment on attachment 373956 [details]
Patch

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

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:336
> +        if architecture == 'mips':
> +            self.command += ['--no-testmasm', '--no-testair', '--no-testb3', '--no-testdfg', '--no-testapi']

We should add arm here too (unsure about aarch64). Also, will this work for both buildbots and EWS (old and new)?
Comment 37 Zhifei Fang 2019-07-12 10:57:46 PDT
(In reply to Guillaume Emont from comment #36)
> Comment on attachment 373956 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373956&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:336
> > +        if architecture == 'mips':
> > +            self.command += ['--no-testmasm', '--no-testair', '--no-testb3', '--no-testdfg', '--no-testapi']
> 
> We should add arm here too (unsure about aarch64). Also, will this work for
> both buildbots and EWS (old and new)?

Here are some my thoughts:

I think JSC team would love to run those test binaries on remote devices, I can disable them for now, since we don't actually run them remotely, I guess we should create another radar to make them run on the remote device, I believe arm support them.
Comment 38 Zhifei Fang 2019-07-12 11:26:44 PDT
Created attachment 374024 [details]
Patch
Comment 39 Zhifei Fang 2019-07-12 11:27:39 PDT
(In reply to Guillaume Emont from comment #36)
> Comment on attachment 373956 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=373956&action=review
> 
> > Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:336
> > +        if architecture == 'mips':
> > +            self.command += ['--no-testmasm', '--no-testair', '--no-testb3', '--no-testdfg', '--no-testapi']
> 
> We should add arm here too (unsure about aarch64). Also, will this work for
> both buildbots and EWS (old and new)?

Did add all those will run remote test arch.
Comment 40 Build Bot 2019-07-12 11:29:23 PDT
Attachment 374024 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Aakash Jain 2019-07-12 11:52:51 PDT
Comment on attachment 374024 [details]
Patch

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

> Tools/ChangeLog:5
> +

Please add corresponding rdar id here.
Comment 42 Zhifei Fang 2019-07-12 12:08:04 PDT
Created attachment 374026 [details]
Patch
Comment 43 Build Bot 2019-07-12 12:10:41 PDT
Attachment 374026 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 44 WebKit Commit Bot 2019-07-12 12:50:12 PDT
Comment on attachment 374026 [details]
Patch

Clearing flags on attachment: 374026

Committed r247393: <https://trac.webkit.org/changeset/247393>
Comment 45 WebKit Commit Bot 2019-07-12 12:50:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 46 Radar WebKit Bug Importer 2019-07-12 12:51:18 PDT
<rdar://problem/53022268>
Comment 47 Guillaume Emont 2019-07-15 04:23:58 PDT
This definitely breaks the arm and mips EWS.
Comment 48 Guillaume Emont 2019-07-15 04:49:39 PDT
(In reply to Guillaume Emont from comment #47)
> This definitely breaks the arm and mips EWS.

Example test output from arm bot (we get the same on the mips bot):
https://webkit-queues.webkit.org/results/12741762

I believe that additional code is needed to disable the binary test report on these archs.
Comment 49 WebKit Commit Bot 2019-07-15 04:55:12 PDT
Re-opened since this is blocked by bug 199797
Comment 50 Zhifei Fang 2019-07-15 10:04:52 PDT
(In reply to Guillaume Emont from comment #48)
> (In reply to Guillaume Emont from comment #47)
> > This definitely breaks the arm and mips EWS.
> 
> Example test output from arm bot (we get the same on the mips bot):
> https://webkit-queues.webkit.org/results/12741762
> 
> I believe that additional code is needed to disable the binary test report
> on these archs.

This is weird, it works well here:

https://build.webkit.org/builders/JSCOnly%20Linux%20MIPS32el%20Release/builds/3310/steps/jscore-test/logs/stdio

Most likely either I miss something, or I miss to restart something.

aakash_jain, any instruction here ?
Comment 51 Guillaume Emont 2019-07-15 10:19:52 PDT
(In reply to Zhifei Fang from comment #50)
> (In reply to Guillaume Emont from comment #48)
> > (In reply to Guillaume Emont from comment #47)
> > > This definitely breaks the arm and mips EWS.
> > 
> > Example test output from arm bot (we get the same on the mips bot):
> > https://webkit-queues.webkit.org/results/12741762
> > 
> > I believe that additional code is needed to disable the binary test report
> > on these archs.
> 
> This is weird, it works well here:
> 
> https://build.webkit.org/builders/JSCOnly%20Linux%20MIPS32el%20Release/
> builds/3310/steps/jscore-test/logs/stdio
> 
> Most likely either I miss something, or I miss to restart something.
> 
> aakash_jain, any instruction here ?

Buildbots and (old) EWS don't use the same scripts. So I think we're in a situation where the buildbots properly ignore the results of tesmasm & co, whereas the EWS looks at them.
I would guess we might need to add code in Tools/Scripts/webkitpy/common/config/ports.py or somewhere around that so that we also pass the --no-testmasm... options for the old EWS.
Alternatively, if this looks unfeasible, I can try to add this to the $JSCTESTS_OPTIONS environment variable of jsc-mips-ews and jsc-armv7-ews.
Comment 52 Zhifei Fang 2019-07-15 10:28:04 PDT
(In reply to Guillaume Emont from comment #51)
> (In reply to Zhifei Fang from comment #50)
> > (In reply to Guillaume Emont from comment #48)
> > > (In reply to Guillaume Emont from comment #47)
> > > > This definitely breaks the arm and mips EWS.
> > > 
> > > Example test output from arm bot (we get the same on the mips bot):
> > > https://webkit-queues.webkit.org/results/12741762
> > > 
> > > I believe that additional code is needed to disable the binary test report
> > > on these archs.
> > 
> > This is weird, it works well here:
> > 
> > https://build.webkit.org/builders/JSCOnly%20Linux%20MIPS32el%20Release/
> > builds/3310/steps/jscore-test/logs/stdio
> > 
> > Most likely either I miss something, or I miss to restart something.
> > 
> > aakash_jain, any instruction here ?
> 
> Buildbots and (old) EWS don't use the same scripts. So I think we're in a
> situation where the buildbots properly ignore the results of tesmasm & co,
> whereas the EWS looks at them.
> I would guess we might need to add code in
> Tools/Scripts/webkitpy/common/config/ports.py or somewhere around that so
> that we also pass the --no-testmasm... options for the old EWS.
> Alternatively, if this looks unfeasible, I can try to add this to the
> $JSCTESTS_OPTIONS environment variable of jsc-mips-ews and jsc-armv7-ews.

I guess I will need to modify the old EWS code as well.
Comment 53 Guillaume Emont 2019-07-15 10:47:31 PDT
(In reply to Zhifei Fang from comment #52)

> I guess I will need to modify the old EWS code as well.

Yes, jsc-mips-ews and jsc-armv7-ews still run the old EWS system.
Comment 54 Zhifei Fang 2019-07-16 15:38:44 PDT
Created attachment 374251 [details]
Patch
Comment 55 Build Bot 2019-07-16 15:39:52 PDT
Attachment 374251 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 Zhifei Fang 2019-07-17 10:36:00 PDT
Created attachment 374306 [details]
Patch
Comment 57 Build Bot 2019-07-17 10:37:26 PDT
Attachment 374306 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 58 Zhifei Fang 2019-07-17 10:59:09 PDT
Created attachment 374307 [details]
Patch
Comment 59 Build Bot 2019-07-17 11:01:39 PDT
Attachment 374307 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 60 Guillaume Emont 2019-07-17 16:07:29 PDT
Comment on attachment 374307 [details]
Patch

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

> Tools/Scripts/webkitpy/common/config/ports.py:259
> +"""
> +    def run_javascriptcore_tests_command(self, build_style=None):
> +        command = self.script_shell_command("run-javascriptcore-tests")
> +        command.append("--no-fail-fast")
> +        command.append("--no-testmasm")
> +        command.append("--no-testair")
> +        command.append("--no-testb3")
> +        command.append("--no-testdfg")
> +        command.append("--no-testapi")
> +        if 'JSCTESTS_OPTIONS' in os.environ:
> +            command += os.environ['JSCTESTS_OPTIONS'].split()
> +        return self._append_build_style_flag(command, build_style)
> +"""

I'm confused as to why the jsc-mips and jsc-armv7 bubbles show green, but things definitely fail there with this version with this code between quotes. A previous version was working for them.
Other than removing the quotes, uou might want to add some conditions on the arch here, since I think that the jsc-i386 and jsc EWS should be able to run these binaries (unless they're already in the new EWS, in which case they wouldn't be affected, but I suspect that at least jsc-i386 is still on the old EWS).
Comment 61 Aakash Jain 2019-07-17 16:21:15 PDT
> I'm confused as to why the jsc-mips and jsc-armv7 bubbles show green
Seems like a bug in old EWS: https://bugs.webkit.org/show_bug.cgi?id=199753

> unless they're already in the new EWS
All the jsc queues are on old EWS currently.
Comment 62 Zhifei Fang 2019-07-18 13:21:59 PDT
jsc failing is because testapi is actually failing, we don't report this in ews before.
Comment 63 Aakash Jain 2019-07-18 13:26:12 PDT
(In reply to Zhifei Fang from comment #62)
> jsc failing is because testapi is actually failing, we don't report this in ews before.
ok, can you please file another bug for that with the details. And is it possible for us to fix the root-cause?
Comment 64 Zhifei Fang 2019-07-22 13:58:09 PDT
Created attachment 374627 [details]
Patch
Comment 65 Build Bot 2019-07-22 14:01:07 PDT
Attachment 374627 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 66 Zhifei Fang 2019-07-23 11:56:12 PDT
Created attachment 374690 [details]
Patch
Comment 67 Build Bot 2019-07-23 11:58:44 PDT
Attachment 374690 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 68 Aakash Jain 2019-07-23 16:48:48 PDT
Comment on attachment 374690 [details]
Patch

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

> Tools/ChangeLog:8
> +        * BuildSlaveSupport/build.webkit.org-config/steps.py:

Please update ChangeLog with the short description of the changes.

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:335
> +        # Currently run-javascriptcore-test doesn't support run those binaries remotely

Nit: run -> running.
Also what does 'those' refer to?
Comment 69 Zhifei Fang 2019-07-23 17:22:29 PDT
Created attachment 374738 [details]
Patch
Comment 70 Build Bot 2019-07-23 17:23:52 PDT
Attachment 374738 [details] did not pass style-queue:


ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:126:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_results_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
ERROR: Tools/BuildSlaveSupport/build.webkit.org-config/steps_unittest.py:132:  [RunJavaScriptCoreTestsTest.test_jsc_stress_failures_with_binary_result_output] Undefined variable 'FAILURE'  [pylint/E0602] [5]
Total errors found: 2 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 71 Aakash Jain 2019-07-23 20:50:16 PDT
Comment on attachment 374738 [details]
Patch

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

lgtm

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:335
> +        # Currently run-javascriptcore-test doesn't support run javascript core test binaries list below remotely

This looks weird/incomplete. Maybe rephrase something like:

# Currently run-javascriptcore-test doesn't support remotely running few javascript core test binaries.
Comment 72 ews-feeder 2019-07-24 10:32:43 PDT
Comment on attachment 374738 [details]
Patch

Rejecting attachment 374738 [details] from commit-queue.

zhifei_fang@apple.com does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.
Comment 73 WebKit Commit Bot 2019-07-24 11:07:07 PDT
Comment on attachment 374738 [details]
Patch

Clearing flags on attachment: 374738

Committed r247781: <https://trac.webkit.org/changeset/247781>
Comment 74 WebKit Commit Bot 2019-07-24 11:07:10 PDT
All reviewed patches have been landed.  Closing bug.