Bug 169538

Summary: Add a tool to update expected.txt files from EWS bot results
Product: WebKit Reporter: youenn fablet <youennf>
Component: Tools / TestsAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cdumez, commit-queue, glenn, lforschler, rniwa, ryanhaddad
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=134767
Attachments:
Description Flags
Patch
none
Update mac-wk1 instead of mac
none
Patch
none
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch for landing
none
Archive of layout-test-results from webkit-cq-02 for mac-elcapitan none

Description youenn fablet 2017-03-12 21:10:21 PDT
This is handy when rebasing large set of tests like W3C imported tests.
Comment 1 youenn fablet 2017-03-12 21:18:35 PDT
Created attachment 304222 [details]
Patch
Comment 2 youenn fablet 2017-03-12 22:26:03 PDT
Created attachment 304226 [details]
Update mac-wk1 instead of mac
Comment 3 Ryosuke Niwa 2017-03-13 20:56:46 PDT
Comment on attachment 304226 [details]
Update mac-wk1 instead of mac

Surely, we should have some tests for this?
Comment 4 Alex Christensen 2017-03-14 10:37:30 PDT
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
Comment 5 youenn fablet 2017-04-09 00:31:25 PDT
Created attachment 306606 [details]
Patch
Comment 6 youenn fablet 2017-04-17 08:01:32 PDT
Ping review
Comment 7 Ryosuke Niwa 2017-04-17 21:09:38 PDT
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.
Comment 8 youenn fablet 2017-04-20 23:33:35 PDT
Created attachment 307700 [details]
Patch
Comment 9 youenn fablet 2017-04-20 23:36:28 PDT
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.
Comment 10 Build Bot 2017-04-21 01:09:36 PDT
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
Comment 11 Build Bot 2017-04-21 01:09:38 PDT
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
Comment 12 Ryosuke Niwa 2017-04-21 13:17:17 PDT
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.
Comment 13 youenn fablet 2017-04-22 22:44:42 PDT
Created attachment 307919 [details]
Patch for landing
Comment 14 youenn fablet 2017-04-22 22:50:49 PDT
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!
Comment 15 WebKit Commit Bot 2017-04-22 23:50:50 PDT
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
Comment 16 WebKit Commit Bot 2017-04-22 23:50:51 PDT
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
Comment 17 WebKit Commit Bot 2017-04-23 12:52:10 PDT
Comment on attachment 307919 [details]
Patch for landing

Clearing flags on attachment: 307919

Committed r215675: <http://trac.webkit.org/changeset/215675>
Comment 18 WebKit Commit Bot 2017-04-23 12:52:12 PDT
All reviewed patches have been landed.  Closing bug.