Bug 122303 – Add a verbose flag to binding tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matthew Hanson]

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.

[Radar WebKit Bug Importer]

<rdar://problem/15146102>

[Matthew Hanson]

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.

[Peter Gal]

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_

[Radar WebKit Bug Importer]

<rdar://problem/15605926
>

[Radar WebKit Bug Importer]

<rdar://problem/15680835
>

[Andreas Kling]

Comment on attachment 213305 [details]
v1 Patch for 122303

r-, please fix the problems pointed out by Peter Gal.

[Tibor Mészáros]

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.

[Peter Gal]

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' :)

[Csaba Osztrogonác]

Comment on attachment 232246 [details]
Patch v2

r-, please fix the problems pointed out by Péter Gál.

[Tibor Mészáros]

Created attachment 232292 [details]
Patch v3

All has been fixed, that Peter mentioned :)

[Csaba Osztrogonác]

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.

[Tibor Mészáros]

Created attachment 232375 [details]
Patch v4

The newline and the extra indentation has been removed.

[WebKit Commit Bot]

Comment on attachment 232375 [details]
Patch v4

Clearing flags on attachment: 232375

Committed r169558: <http://trac.webkit.org/changeset/169558>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.