Bug 122303

Summary: Add a verbose flag to binding tests
Product: WebKit Reporter: Matthew Hanson <matthew_hanson>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ap, commit-queue, dpranke, glenn, lforschler, ossy, webkit-bug-importer
Priority: P4 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
v1 Patch for 122303
kling: review-, kling: commit-queue-
Patch v2
ossy: review-, ossy: commit-queue-
Patch v3
ossy: review-, ossy: commit-queue-
Patch v4 none

Description Matthew Hanson 2013-10-03 15:11:41 PDT
While investigating Bug 122017, I found that positive BindingTests results are unconditionally printed to stdout.  It is often desirable to suppress the output of these test cases since they are run often and expected to fail rarely.

The BindingTests class is defined in Tools/Scripts/webkitpy/bindings/main.py.
Comment 1 Radar WebKit Bug Importer 2013-10-03 15:12:04 PDT
<rdar://problem/15146102>
Comment 2 Matthew Hanson 2013-10-03 15:36:33 PDT
Created attachment 213305 [details]
v1 Patch for 122303

Added a verbose keyword argument to the BindingsTests constructor.
Verbose mode, which is on by default, prints positive results to stdout as before.
When the verbose flag is set to false, positive results are not printed at all.
Comment 3 Peter Gal 2013-10-18 05:13:41 PDT
Comment on attachment 213305 [details]
v1 Patch for 122303

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

The 'run-bindings-test' script should be adapted for this change. Without that there is no way to disable it from command line.

> Tools/Scripts/webkitpy/bindings/main.py:117
> +                if self.verbose is True:

A simple 'if self.verbose' is enough. Also 'is' is for identity checking see http://docs.python.org/2/library/operator.html#operator.is_
Comment 4 Radar WebKit Bug Importer 2013-12-06 11:25:59 PST
<rdar://problem/15605926
>
Comment 5 Radar WebKit Bug Importer 2013-12-17 11:24:20 PST
<rdar://problem/15680835
>
Comment 6 Andreas Kling 2014-02-05 17:38:02 PST
Comment on attachment 213305 [details]
v1 Patch for 122303

r-, please fix the problems pointed out by Peter Gal.
Comment 7 Tibor Mészáros 2014-05-29 07:08:46 PDT
Created attachment 232246 [details]
Patch v2

I had finished Matthew's patch. The following changes where made:
- Added --no-verbose option to the run-bindigs-test.
- Changed "if self.verbose is True" to "self.verbose" as Peter Gal wanted.
Comment 8 Peter Gal 2014-05-29 09:02:15 PDT
Comment on attachment 232246 [details]
Patch v2

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

> Tools/Scripts/run-bindings-tests:47
> +    if "--no-verbose" in argv:
> +        verbose = False
> +    else:
> +        verbose = True

We should make this a bit simpler a one-liner, like in the line 42. Eg.: verbose = not "--no-verbose" in argv

> Tools/Scripts/run-bindings-tests:52
> +        'GObject',

I don't see any point of this change, why is this needed?

> Tools/Scripts/webkitpy/bindings/main.py:117
>              else:
> -                print 'PASS: (%s) %s' % (generator, output_file)
> +                if self.verbose:

Looking at this know I realize we can merge the 'else' and 'if self.verbose' into an 'elsif self.verbose' :)
Comment 9 Csaba Osztrogonác 2014-05-29 09:03:55 PDT
Comment on attachment 232246 [details]
Patch v2

r-, please fix the problems pointed out by Péter Gál.
Comment 10 Tibor Mészáros 2014-05-30 05:13:50 PDT
Created attachment 232292 [details]
Patch v3

All has been fixed, that Peter mentioned :)
Comment 11 Csaba Osztrogonác 2014-05-30 05:47:09 PDT
Comment on attachment 232292 [details]
Patch v3

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

> Tools/Scripts/run-bindings-tests:43
> +    

This extra newline is unnecessary.

> Tools/Scripts/webkitpy/bindings/main.py:117
> +            elif self.verbose:
> +                    print 'PASS: (%s) %s' % (generator, output_file)

Please remove this extra indentation.
Comment 12 Tibor Mészáros 2014-06-02 04:14:50 PDT
Created attachment 232375 [details]
Patch v4

The newline and the extra indentation has been removed.
Comment 13 WebKit Commit Bot 2014-06-02 23:52:00 PDT
Comment on attachment 232375 [details]
Patch v4

Clearing flags on attachment: 232375

Committed r169558: <http://trac.webkit.org/changeset/169558>
Comment 14 WebKit Commit Bot 2014-06-02 23:52:04 PDT
All reviewed patches have been landed.  Closing bug.