Bug 204386 – run-javascriptcore-tests: Hide the output of binaries behind --verbose

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Aakash pointed out that we might have way too many logs coming out of run-javascriptcore-tests by default. We should gate some of these logs on the --verbose flag.

[Jonathan Bedard]

Created attachment 383921 [details]
Patch

[Aakash Jain]

Comment on attachment 383921 [details]
Patch

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

> Tools/Scripts/run-javascriptcore-tests:585
> +            print "$subTestName failed\n" if ($failureFlag && !$verbose);

Shouldn't we print the failures irrespective of verbose flag. Hiding failure might make it difficult to debug those failures. Especially important for EWS, where engineers can't change the flag.

[Jonathan Bedard]

(In reply to Aakash Jain from comment #2)
> Comment on attachment 383921 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383921&action=review
> 
> > Tools/Scripts/run-javascriptcore-tests:585
> > +            print "$subTestName failed\n" if ($failureFlag && !$verbose);
> 
> Shouldn't we print the failures irrespective of verbose flag. Hiding failure
> might make it difficult to debug those failures. Especially important for
> EWS, where engineers can't change the flag.

If we're verbose, we would have printed the error line already on line 522, I was thinking that we wouldn't want to duplicate the printing.

I don't feel super strongly about it either way, though.

[Aakash Jain]

Comment on attachment 383921 [details]
Patch

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

>>> Tools/Scripts/run-javascriptcore-tests:585
>>> +            print "$subTestName failed\n" if ($failureFlag && !$verbose);
>> 
>> Shouldn't we print the failures irrespective of verbose flag. Hiding failure might make it difficult to debug those failures. Especially important for EWS, where engineers can't change the flag.
> 
> If we're verbose, we would have printed the error line already on line 522, I was thinking that we wouldn't want to duplicate the printing.
> 
> I don't feel super strongly about it either way, though.

That's ok either way.

But why is this print inside if reportData?

[Jonathan Bedard]

Comment on attachment 383921 [details]
Patch

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

>>>> Tools/Scripts/run-javascriptcore-tests:585
>>>> +            print "$subTestName failed\n" if ($failureFlag && !$verbose);
>>> 
>>> Shouldn't we print the failures irrespective of verbose flag. Hiding failure might make it difficult to debug those failures. Especially important for EWS, where engineers can't change the flag.
>> 
>> If we're verbose, we would have printed the error line already on line 522, I was thinking that we wouldn't want to duplicate the printing.
>> 
>> I don't feel super strongly about it either way, though.
> 
> That's ok either way.
> 
> But why is this print inside if reportData?

This is handling the case where we somehow duplicate a test name when parsing binary output. This is possible because we don't have great standards for how JSC binary tests are organized.

That line of code is basically saying 'If we don't already have a result for this test, then'

[WebKit Commit Bot]

Comment on attachment 383921 [details]
Patch

Clearing flags on attachment: 383921

Committed r252781: <https://trac.webkit.org/changeset/252781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/57432918>