[webkit-reviews] review granted: [Bug 182584] webkit-patch suggest-reviewers dies with AttributeError: 'NoneType' object has no attribute 'revision' : [Attachment 333377] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 182584: webkit-patch suggest-reviewers dies with AttributeError: 'NoneType'
object has no attribute 'revision'
https://bugs.webkit.org/show_bug.cgi?id=182584

Attachment 333377: Patch

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




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

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

> Tools/Scripts/webkitpy/common/checkout/checkout.py:140
> +	   return set(map(self.commit_info_for_revision, revisions)) -
set([None])

Please add a comment above this line that explains why a None can be in the
set. Maybe something like:

# Remove a None entry from the set. This can happen if a revision does have an
associated ChangeLog entry (e.g. r185745).

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:431
> +		   return [-1]	# Will return None for revision information.

This code is disingenuous and the comment is not precise. {Git,
SVN}.revisions_changing_file() always returns a list of valid revisions. For
the purposes of testings the number that we pick for the revision serves as a
unique identifier so that when mock_changelog_entries_for_revision() is called
it knows what mock value to return. It is always best to make the unit test and
mock code as representable as possible to the real environment to ensure that
we are exercising real-world code paths and not inadvertently testing some
quirky, unit-test-specific code path that may make the unit test pass, but is
not representative of the real-world code path that we intended to test.

We should return a list with some unique revision number here, say 27, and then
have mock_changelog_entries_for_revision() return the empty list for revision
27. I do not think a comment is necessary. If you are adamant about having a
comment then the comment should reference
"mock_changelog_entries_for_revision()".

> Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:436
> +	   checkout._scm.changed_files = lambda git_commit: ["file1", "file2",
"relative/path/ChangeLog", 'file_with_empty_changelog']

We should take the opportunity to fix up the style of this line since we are
modifying it. Please change all double quotes to single quotes.