[webkit-reviews] review granted: [Bug 169538] Add a tool to update expected.txt files from EWS bot results : [Attachment 307700] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 169538: Add a tool to update expected.txt files from EWS bot results
https://bugs.webkit.org/show_bug.cgi?id=169538

Attachment 307700: Patch

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




--- Comment #12 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 307700
  --> https://bugs.webkit.org/attachment.cgi?id=307700
Patch

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

r=me if all my comments below are addressed.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:66
> +    parser.add_argument('-s', '--is-attachment-specific',
dest='is_attachment_specific', action='store_true', default=False,
help='Whether generic expected files should be updated or not')

I don't think "specific" is adequate description here because it doesn't say
for what it is specific.
It should be either platform-specific (preferred) or port-specific.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:81
> +	       for attachment in
attachment_fetcher.fetch_bug(bugzilla_id).attachments():
> +		   bot_type = self._bot_type(attachment)
> +		   if bot_type:
> +		       self.attachments[bot_type] = attachment

In which order are we traversing attachment?
It's possible for the same EWS to upload multiple attachments when there are
multiple patches being uploaded.
And we probably want to be using the latest attachment.
We should add a test for that case.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:122
> +    def _update_from_secondary_attachment(self, attachment, bot_type):

Again, it's not clear what "secondary" means. We should at least match with the
names used in the options.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:141
> +	   processed_attachment = self.generic_attachment
> +	   for bot_type, attachment in self.attachments.iteritems():
> +	       if attachment == self.generic_attachment:
> +		   continue
> +	       processed_attachment = True
> +	       self._update_from_secondary_attachment(attachment, bot_type)

This is very C/C++ looking code.
It'd be better if you just used filter to get rid of generic_attachment using
"list" comprehension:
platform_specific_attachments = {bot_type: attachment for bot_type, attachment
in self.attachments.iteritems() if bot_type != self.generic_attachment}

Another options is to pop the generic_attachment from the list of attachments
in the constructor as in:
self.generic_attachment = self.attachments.pop("mac-wk2")
That might be cleaner since we're already walking over attachments there.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:142
> +	   if not processed_attachment:

That way, this check can simply be: not self.generic_attachment or not
platform_specific_attachments.