Bug 66838 - Extract reference links from reftest test file
Summary: Extract reference links from reftest test file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 66837
  Show dependency treegraph
 
Reported: 2011-08-23 22:33 PDT by Ai Makabi
Modified: 2011-08-31 01:18 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.39 KB, patch)
2011-08-24 22:01 PDT, Ai Makabi
no flags Details | Formatted Diff | Diff
Patch (7.50 KB, patch)
2011-08-30 22:42 PDT, Ai Makabi
no flags Details | Formatted Diff | Diff
Patch (7.65 KB, patch)
2011-08-31 00:18 PDT, Ai Makabi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ai Makabi 2011-08-23 22:33:46 PDT
Extract reference links from reftest test file
Comment 1 Ai Makabi 2011-08-24 22:01:54 PDT
Created attachment 105129 [details]
Patch
Comment 2 Shinichiro Hamaji 2011-08-24 22:53:29 PDT
Comment on attachment 105129 [details]
Patch

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

This patch looks good, but putting r- for now as there are some nitpicks.

If you didn't yet, please run ./Tools/Scripts/check-webkit-style to see if there are style issues.

> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link.py:42
> +            if attrs["rel"] == "match":

I think this will raise an error (KeyError) if the input html contains <link> without rel attribute.

> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link.py:48
> +def get_reference_link(html):

Could you write a docstring for this? I think it should explain the argument (is this a string or a file?) and the return value (a tuple)

> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:28
> +import extract_reference_link

from webkitpy.layout_tests.reftests import extract_reference_link ?

I guess this won't pass ./Tools/Scripts/test-webkit-scripts , but I'm not sure.

> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:29
> +

BTW, we usually put imports in alphabetical order.

> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:61
> +                         ["red-box-notref.xht", "red-box-notref.xht"])

It might be nice if we have another test function which exercises error cases. For example, I'd test <link> without rel, unclosed <link> (what will happen if the input is broken as an HTML...?), etc.
Comment 3 Ai Makabi 2011-08-30 22:42:04 PDT
Created attachment 105742 [details]
Patch
Comment 4 Ai Makabi 2011-08-30 22:45:28 PDT
Comment on attachment 105129 [details]
Patch

Thank you for the review.

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

>> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link.py:42
>> +            if attrs["rel"] == "match":
> 
> I think this will raise an error (KeyError) if the input html contains <link> without rel attribute.

Done.

>> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link.py:48
>> +def get_reference_link(html):
> 
> Could you write a docstring for this? I think it should explain the argument (is this a string or a file?) and the return value (a tuple)

Done.

>> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:28
>> +import extract_reference_link
> 
> from webkitpy.layout_tests.reftests import extract_reference_link ?
> 
> I guess this won't pass ./Tools/Scripts/test-webkit-scripts , but I'm not sure.

Done.

>> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:29
>> +
> 
> BTW, we usually put imports in alphabetical order.

Done.

>> Tools/Scripts/webkitpy/layout_tests/reftests/extract_reference_link_unittest.py:61
>> +                         ["red-box-notref.xht", "red-box-notref.xht"])
> 
> It might be nice if we have another test function which exercises error cases. For example, I'd test <link> without rel, unclosed <link> (what will happen if the input is broken as an HTML...?), etc.

Done.
Comment 5 Kent Tamura 2011-08-30 23:39:34 PDT
I guess EWSs are purple because of the empty __init__.py.  Would you add a comment in __init__.py please?
Comment 6 Ai Makabi 2011-08-31 00:18:11 PDT
Created attachment 105752 [details]
Patch
Comment 7 Ai Makabi 2011-08-31 00:18:51 PDT
Done.

(In reply to comment #5)
> I guess EWSs are purple because of the empty __init__.py.  Would you add a comment in __init__.py please?
Comment 8 Shinichiro Hamaji 2011-08-31 00:34:35 PDT
Comment on attachment 105752 [details]
Patch

Looks good. Thanks for addressing my comments.
Comment 9 WebKit Review Bot 2011-08-31 01:18:03 PDT
Comment on attachment 105752 [details]
Patch

Clearing flags on attachment: 105752

Committed r94160: <http://trac.webkit.org/changeset/94160>
Comment 10 WebKit Review Bot 2011-08-31 01:18:07 PDT
All reviewed patches have been landed.  Closing bug.