run-javascriptcore-tests won't report test results for testmasm, testair, testb3, testdfg and test api

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zhifei Fang]

run-javascript-core-test won't report json results for testmasm, testair, testb3, testdfg and test api without fail fast

[Zhifei Fang]

Created attachment 373451 [details]
Patch

[EWS Watchlist]

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

[Aakash Jain]

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.

[Zhifei Fang]

Created attachment 373665 [details]
Patch

[Aakash Jain]

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.

[Zhifei Fang]

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

[Zhifei Fang]

Created attachment 373779 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Zhifei Fang]

Created attachment 373850 [details]
Patch

[EWS Watchlist]

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.

[Aakash Jain]

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?

[Zhifei Fang]

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

[Aakash Jain]

Comment on attachment 373850 [details]
Patch

looks fine to me, but would prefer if jbedard can also have a look.

[Jonathan Bedard]

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?

[Jonathan Bedard]

Can we see an example of old output vs new output?

[Zhifei Fang]

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

[Zhifei Fang]

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

[Jonathan Bedard]

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.

[Zhifei Fang]

Created attachment 373866 [details]
Patch

[Zhifei Fang]

(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

[EWS Watchlist]

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.

[Jonathan Bedard]

jsc-mips is failing.

Is this because we were ignoring such failures earlier? Is this an existing failure?

[Zhifei Fang]

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

[Zhifei Fang]

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

[Jonathan Bedard]

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

[Jonathan Bedard]

Comment on attachment 373866 [details]
Patch

Zhifei says that the problem jsc-mips has surfaced may be a real issue.

[Zhifei Fang]

(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

[Guillaume Emont]

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

[Guillaume Emont]

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

[Jonathan Bedard]

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

[Zhifei Fang]

Created attachment 373956 [details]
Patch

[EWS Watchlist]

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.

[Zhifei Fang]

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

[Guillaume Emont]

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

[Guillaume Emont]

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)?

[Zhifei Fang]

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

[Zhifei Fang]

Created attachment 374024 [details]
Patch

[Zhifei Fang]

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

[EWS Watchlist]

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.

[Aakash Jain]

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.

[Zhifei Fang]

Created attachment 374026 [details]
Patch

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 374026 [details]
Patch

Clearing flags on attachment: 374026

Committed r247393: <https://trac.webkit.org/changeset/247393>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53022268>

[Guillaume Emont]

This definitely breaks the arm and mips EWS.

[Guillaume Emont]

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 199797

[Zhifei Fang]

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

[Guillaume Emont]

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

[Zhifei Fang]

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

[Guillaume Emont]

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

[Zhifei Fang]

Created attachment 374251 [details]
Patch

[EWS Watchlist]

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.

[Zhifei Fang]

Created attachment 374306 [details]
Patch

[EWS Watchlist]

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.

[Zhifei Fang]

Created attachment 374307 [details]
Patch

[EWS Watchlist]

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.

[Guillaume Emont]

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

[Aakash Jain]

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

[Zhifei Fang]

jsc failing is because testapi is actually failing, we don't report this in ews before.

[Aakash Jain]

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

[Zhifei Fang]

Created attachment 374627 [details]
Patch

[EWS Watchlist]

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.

[Zhifei Fang]

Created attachment 374690 [details]
Patch

[EWS Watchlist]

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.

[Aakash Jain]

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?

[Zhifei Fang]

Created attachment 374738 [details]
Patch

[EWS Watchlist]

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.

[Aakash Jain]

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.

[EWS]

Comment on attachment 374738 [details]
Patch

Rejecting attachment 374738 [details] from commit-queue.

[e-mail] 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.

[WebKit Commit Bot]

Comment on attachment 374738 [details]
Patch

Clearing flags on attachment: 374738

Committed r247781: <https://trac.webkit.org/changeset/247781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.