Bug 87802 - webkitpy: add support for an ordered dict of test expectations
Summary: webkitpy: add support for an ordered dict of test expectations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 65834
  Show dependency treegraph
 
Reported: 2012-05-29 18:14 PDT by Dirk Pranke
Modified: 2012-06-06 17:49 PDT (History)
5 users (show)

See Also:


Attachments
Patch (11.24 KB, patch)
2012-05-29 18:19 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
actually test on python2.6, fix names (11.24 KB, patch)
2012-05-29 18:28 PDT, Dirk Pranke
ojan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2012-05-29 18:14:48 PDT
webkitpy: add support for an ordered dict of test expectations
Comment 1 Dirk Pranke 2012-05-29 18:19:52 PDT
Created attachment 144666 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-29 18:21:52 PDT
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.
Comment 3 Dirk Pranke 2012-05-29 18:28:38 PDT
Created attachment 144667 [details]
actually test on python2.6, fix names
Comment 4 Ojan Vafai 2012-05-29 20:08:40 PDT
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.
Comment 5 Dirk Pranke 2012-05-30 13:55:42 PDT
(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 6 Ojan Vafai 2012-05-30 14:30:32 PDT
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.
Comment 7 Dirk Pranke 2012-06-06 17:49:20 PDT
Committed r119654: <http://trac.webkit.org/changeset/119654>