Bug 169538 – Add a tool to update expected.txt files from EWS bot results

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[youenn fablet]

This is handy when rebasing large set of tests like W3C imported tests.

[youenn fablet]

Created attachment 304222 [details]
Patch

[youenn fablet]

Created attachment 304226 [details]
Update mac-wk1 instead of mac

[Ryosuke Niwa]

Comment on attachment 304226 [details]
Update mac-wk1 instead of mac

Surely, we should have some tests for this?

[Alex Christensen]

Comment on attachment 304226 [details]
Update mac-wk1 instead of mac

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

> Tools/Scripts/update-test-expectations-from-bugzilla:3
> +# Copyright (C) 2017 Apple INC. All rights reserved.

INC

[youenn fablet]

Created attachment 306606 [details]
Patch

[youenn fablet]

Ping review

[Ryosuke Niwa]

Comment on attachment 306606 [details]
Patch

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

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:64
> +    parser.add_argument('-b', '--bug', dest='bug_id', default=None, help='Bug ID to which get expectations from bugzilla')

Do we really need to say default=None? I thought None was default anyway.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:66
> +    parser.add_argument('-m', '--main', dest='is_attachment_main', action='store_true', default=False, help='Whether the attachment should apply to main expected files or platform specific ones')

I don't think we want to invent new terminology like "main". We've got too many concepts in the layout tests as is.
I think we should call this --no-platform or something.
But why do we need this concept at all given we know mac-wk2 is the "default" platform anyway?
It seems rather confusing to have this option at all.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:70
> +    if (not len(args) and not options.attachment_id):

Nit: No need for parentheses
Also, no need to call len. Just "not args" would suffice and matches the WebKit's style guideline.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:113
> +    def _actual_file(self, test_name):
> +        return self.filesystem.splitext(test_name)[0] + '-actual.txt'
> +
> +    def _expected_file(self, test_name, bot_type=None):
> +        if not bot_type:
> +            return self.filesystem.join(self.layout_test_repository, self.filesystem.splitext(test_name)[0] + '-expected.txt')
> +        return self.filesystem.join(self.layout_test_repository, "platform", bot_type, self.filesystem.splitext(test_name)[0] + '-expected.txt')

We should expose a new function get actual and expected file names in TestResultWriter
webkitpy/layout_tests/controllers/test_result_writer.py instead of duplicating the suffixes here.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:117
> +        zip_file = self.unzipper.unzip(attachment.contents()) if self.unzipper else zipfile.ZipFile(io.BytesIO(attachment.contents()))

Instead of falling back to zipfile.ZipFile, the default argument to __init__ should just be zipfile.ZipFile.
Alternatively, it can fallback to a lambda inside __init__.

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

We should be sharing code with single_test_runner._save_baseline_data.

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

Ditto.

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:151
> +    options, args = parse_args(_argv)

I think it's better to have parse_args merged into this function so that

> Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:155
> +    updater = TestExpectationUpdater(Host(), args[0], options.attachment_id, options.is_attachment_main)

We know what args[0] is, and other options in the same function.

[youenn fablet]

Created attachment 307700 [details]
Patch

[youenn fablet]

Thanks for the review.
I updated the patch accordingly.
See inline for more.

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 306606 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306606&action=review
> 
> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:64
> > +    parser.add_argument('-b', '--bug', dest='bug_id', default=None, help='Bug ID to which get expectations from bugzilla')
> 
> Do we really need to say default=None? I thought None was default anyway.

I simplified the options a bit.

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:66
> > +    parser.add_argument('-m', '--main', dest='is_attachment_main', action='store_true', default=False, help='Whether the attachment should apply to main expected files or platform specific ones')
> 
> I don't think we want to invent new terminology like "main". We've got too
> many concepts in the layout tests as is.
> I think we should call this --no-platform or something.
> But why do we need this concept at all given we know mac-wk2 is the
> "default" platform anyway?
> It seems rather confusing to have this option at all.

Reused generic/specific terminology

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:113
> > +    def _actual_file(self, test_name):
> > +        return self.filesystem.splitext(test_name)[0] + '-actual.txt'
> > +
> > +    def _expected_file(self, test_name, bot_type=None):
> > +        if not bot_type:
> > +            return self.filesystem.join(self.layout_test_repository, self.filesystem.splitext(test_name)[0] + '-expected.txt')
> > +        return self.filesystem.join(self.layout_test_repository, "platform", bot_type, self.filesystem.splitext(test_name)[0] + '-expected.txt')
> 
> We should expose a new function get actual and expected file names in
> TestResultWriter
> webkitpy/layout_tests/controllers/test_result_writer.py instead of
> duplicating the suffixes here.

Done

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:117
> > +        zip_file = self.unzipper.unzip(attachment.contents()) if self.unzipper else zipfile.ZipFile(io.BytesIO(attachment.contents()))
> 
> Instead of falling back to zipfile.ZipFile, the default argument to __init__
> should just be zipfile.ZipFile.
> Alternatively, it can fallback to a lambda inside __init__.

OK

> > Tools/Scripts/webkitpy/common/net/bugzilla/test_expectation_updater.py:122
> > +    def _update_from_main_attachment(self, attachment):
> 
> We should be sharing code with single_test_runner._save_baseline_data.

Logging is made a bit differently. Added a FIXME.

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

Processing is done a bit differently. Added a FIXME.

[Build Bot]

Comment on attachment 307700 [details]
Patch

Attachment 307700 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3574785

New failing tests:
webrtc/datachannel/bufferedAmountLowThreshold.html

[Build Bot]

Created attachment 307709 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Ryosuke Niwa]

Comment on attachment 307700 [details]
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.

[youenn fablet]

Created attachment 307919 [details]
Patch for landing

[youenn fablet]

Thanks for the review.

(In reply to Ryosuke Niwa from comment #12)
> Comment on attachment 307700 [details]
> 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.

platform-specific seems fine.

> > 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?

In the order provided by bugzilla, latest patches happen at the end.

> 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.

Added a test for it.
Current script requires to obsolete past results otherwise we may sometimes end up updating expectations based on obsolete results.

Maybe webkit-patch should offer a way to obsolete all previous results when uploading a new patch.


> > 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.

Done

> > 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.

Pop seems much cleaner, thanks!

[WebKit Commit Bot]

Comment on attachment 307919 [details]
Patch for landing

Rejecting attachment 307919 [details] from commit-queue.

New failing tests:
webrtc/datachannel/basic.html
Full output: http://webkit-queues.webkit.org/results/3587903

[WebKit Commit Bot]

Created attachment 307921 [details]
Archive of layout-test-results from webkit-cq-02 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-02  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[WebKit Commit Bot]

Comment on attachment 307919 [details]
Patch for landing

Clearing flags on attachment: 307919

Committed r215675: <http://trac.webkit.org/changeset/215675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.