Bug 169538

Created attachment 304222 [details] Patch

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

Comment on attachment 304226 [details] Update mac-wk1 instead of mac Surely, we should have some tests for this?

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

Created attachment 306606 [details] Patch

Ping review

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.

Created attachment 307700 [details] Patch

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

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

Created attachment 307919 [details] Patch for landing

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

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 on attachment 307919 [details] Patch for landing Clearing flags on attachment: 307919 Committed r215675: <http://trac.webkit.org/changeset/215675>

All reviewed patches have been landed. Closing bug.