[webkit-reviews] review denied: [Bug 188185] test wkbi with webkit-patch upload : [Attachment 346118] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Lucas Forschler
<lforschler at apple.com>'s request for review:
Bug 188185: test wkbi with webkit-patch upload
https://bugs.webkit.org/show_bug.cgi?id=188185

Attachment 346118: Patch

https://bugs.webkit.org/attachment.cgi?id=346118&action=review




--- Comment #3 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 346118
  --> https://bugs.webkit.org/attachment.cgi?id=346118
Patch

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

We should add tests for this change. Use the _unittests.py files in
Tools/Scripts/webkitpy/tool/steps as examples. You can run the WebKitPy tests
using script test-webkitpy.

> Tools/ChangeLog:3
> +	   test wkbi with webkit-patch upload

This title does not make sense. I think you meant, CC WebKit Bug Importer when
uploading a patch.

> Tools/ChangeLog:8
> +

We should explain that the purpose of this change is to automatically CC WebKit
Bug Importer and add keyword InRadar on Bugzilla bugs when a patch is uploaded
to the bug that contain a radar URL.

> Tools/Scripts/webkitpy/tool/steps/addwkbi.py:27
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +#	 * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#	 * Redistributions in binary form must reproduce the above
> +# copyright notice, this list of conditions and the following disclaimer
> +# in the documentation and/or other materials provided with the
> +# distribution.
> +#	 * Neither the name of Google Inc. nor the names of its
> +# contributors may be used to endorse or promote products derived from
> +# this software without specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> +# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> +# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> +# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> +# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> +# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> +# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> +# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

This is not the correct license block. Please use the Apple license block text.
The text is available at
<https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/LICENSE-APPLE>.

> Tools/Scripts/webkitpy/tool/steps/addwkbi.py:46
> +    def GetRadarLinkFromDiff(self, diff):

This name does not match our code style convention of using all lowercase with
underscores separating words.

> Tools/Scripts/webkitpy/tool/steps/addwkbi.py:58
> +	   matcher =
re.compile(r'(<?rdar:\/\/problems?\/)?(?P<radar_id>-?\d{7,})>?')
> +	   match = matcher.search(diff)
> +
> +	   if not match:
> +		   return None
> +
> +	   id = int(match.group('radar_id'))
> +	   if id < 0:
> +	       return None
> +
> +	   link = '<rdar://problem/{}>'.format(id)
> +	   return link

This code finds the first radar URL in the entire diff and returns it. This
could cause us to falsely associate the bug with a radar URL that appears in an
added or moved comment. It seems sufficient to look for the radar URL only in
the added ChangeLog entries. Toward this we can make use of DiffParser to parse
a patch and the ChangeLog parsing logic in file
<https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/checkout/c
hangelog.py?rev=225733>. You may find inspiration for how to do this logic by
looking at the class ValidateChangeLogs:
<https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/steps/valida
techangelogs.py?rev=150404>.