Bug 87802

Created attachment 144666 [details] Patch

Attachment 144666 [details] did not pass style-queue: Traceback (most recent call last): File "/mnt/git/webkit-style-queue/Tools/Scripts/webkit-patch", line 44, in <module> from webkitpy.tool.main import WebKitPatch File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/tool/main.py", line 37, in <module> from webkitpy.common.host import Host File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/host.py", line 38, in <module> from webkitpy.common.net.buildbot.chromiumbuildbot import ChromiumBuildBot File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/common/net/buildbot/chromiumbuildbot.py", line 32, in <module> from webkitpy.layout_tests.port.builders import builder_path_from_name File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/__init__.py", line 34, in <module> from base import Port # It's possible we don't need to export this virtual baseclass outside the module. File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/layout_tests/port/base.py", line 44, in <module> from webkitpy.third_party.ordered_dict import OrderedDict ImportError: No module named third_party.ordered_dict If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 144667 [details] actually test on python2.6, fix names

Comment on attachment 144667 [details] actually test on python2.6, fix names View in context: https://bugs.webkit.org/attachment.cgi?id=144667&action=review > Tools/Scripts/webkitpy/layout_tests/port/base.py:895 > + """Returns an OrderedDict of name -> expectations strings. The names > + are expected to be (but not required to be) paths in the filesystem. > + If the name is a path, the file can be considered updatable for things > + like rebaselining, so don't use names that are paths if they're not paths. > + Generally speaking the ordering should be files in the filesystem in > + cascade order (test_expectations.txt followed by Skipped, if the port > + honors both formats), then any built-in expectations (e.g., from compile-time > + exclusions), then --additional-expectations options.""" This has more information than is useful I think. Something like the following would be better IMO: """Returns an OrderedDict of name -> expectations strings. If that set of expectations corresponds to a file, name is the path to the file. Files are ordered in cascade order test_expectations.txt, then cascade order Skipped, then compile-time exclusions, then --additional-expectations.""" Up to you, but all the business about rebaselining seems extraneous to me. I'm not really sure we need any of this comment actually. It's all "what" and no "why". > Tools/Scripts/webkitpy/layout_tests/port/base.py:897 > + overrides = OrderedDict() overrides is a weird name for this.

(In reply to comment #4) > (From update of attachment 144667 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144667&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/base.py:895 > > + """Returns an OrderedDict of name -> expectations strings. The names > > + are expected to be (but not required to be) paths in the filesystem. > > + If the name is a path, the file can be considered updatable for things > > + like rebaselining, so don't use names that are paths if they're not paths. > > + Generally speaking the ordering should be files in the filesystem in > > + cascade order (test_expectations.txt followed by Skipped, if the port > > + honors both formats), then any built-in expectations (e.g., from compile-time > > + exclusions), then --additional-expectations options.""" > > This has more information than is useful I think. Something like the following would be better IMO: > """Returns an OrderedDict of name -> expectations strings. If that set of expectations corresponds to a file, name is the path to the file. Files are ordered in cascade order test_expectations.txt, then cascade order Skipped, then compile-time exclusions, then --additional-expectations.""" > > Up to you, but all the business about rebaselining seems extraneous to me. I'm not really sure we need any of this comment actually. It's all "what" and no "why". > I'll prune it down as you suggest. I added all the comments because this is a base class method that I expect to be overridden and it's hard to see what the contract for the method is supposed to be (it does a fair number of things). I agree that it's all "what" but that didn't seem bad in this case. I tried to add unit tests for some of the things in the contract, but it wasn't clear to me (yet at least) how to add tests for all of them. Any ideas for other ways to check these things? > > Tools/Scripts/webkitpy/layout_tests/port/base.py:897 > > + overrides = OrderedDict() > > overrides is a weird name for this.

Comment on attachment 144667 [details] actually test on python2.6, fix names View in context: https://bugs.webkit.org/attachment.cgi?id=144667&action=review >>> Tools/Scripts/webkitpy/layout_tests/port/base.py:895 >>> + exclusions), then --additional-expectations options.""" >> >> This has more information than is useful I think. Something like the following would be better IMO: >> """Returns an OrderedDict of name -> expectations strings. If that set of expectations corresponds to a file, name is the path to the file. Files are ordered in cascade order test_expectations.txt, then cascade order Skipped, then compile-time exclusions, then --additional-expectations.""" >> >> Up to you, but all the business about rebaselining seems extraneous to me. I'm not really sure we need any of this comment actually. It's all "what" and no "why". > > I'll prune it down as you suggest. I added all the comments because this is a base class method that I expect to be overridden and it's hard to see what the contract for the method is supposed to be (it does a fair number of things). I agree that it's all "what" but that didn't seem bad in this case. I tried to add unit tests for some of the things in the contract, but it wasn't clear to me (yet at least) how to add tests for all of them. Any ideas for other ways to check these things? No. I think it's fine. You'll add tests when you add the other expectations.

Committed r119654: <http://trac.webkit.org/changeset/119654>