Bug 64386 - Extract model-like TestExpectationLine and TestExpectationFile from TestExpectations.
Summary: Extract model-like TestExpectationLine and TestExpectationFile from TestExpec...
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: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 64385
  Show dependency treegraph
 
Reported: 2011-07-12 13:24 PDT by Dimitri Glazkov (Google)
Modified: 2011-07-13 08:49 PDT (History)
1 user (show)

See Also:


Attachments
WIP: Needs tests. (14.28 KB, patch)
2011-07-12 13:26 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
With tests. (23.33 KB, patch)
2011-07-12 15:34 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Much better. (23.14 KB, patch)
2011-07-12 16:10 PDT, Dimitri Glazkov (Google)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-07-12 13:24:42 PDT
Extract model-like TestExpectationLine and TestExpectationFile from TestExpectations.
Comment 1 Dimitri Glazkov (Google) 2011-07-12 13:26:45 PDT
Created attachment 100553 [details]
WIP: Needs tests.
Comment 2 Dimitri Glazkov (Google) 2011-07-12 15:34:24 PDT
Created attachment 100575 [details]
With tests.
Comment 3 Dimitri Glazkov (Google) 2011-07-12 15:39:18 PDT
Comment on attachment 100575 [details]
With tests.

this is wrong. fixing.
Comment 4 Dimitri Glazkov (Google) 2011-07-12 16:10:34 PDT
Created attachment 100579 [details]
Much better.
Comment 5 Adam Barth 2011-07-12 16:24:46 PDT
Comment on attachment 100579 [details]
Much better.

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

This looks fantastic.  Some detail nits below.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208
> +        comment_index = line.find("//")
> +        comment = ''
> +        if comment_index == -1:
> +            comment_index = len(line)
> +            comment = None
> +        else:
> +            comment = line[comment_index + 2:]

You can't split on substrings in python?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:217
> +            return (None, None, None, None)

Consider using a dictionary instead of a tuple.  That's more self-documenting / self-error-correcting.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:233
> +class TestExpectationLine:

Seems like _split_expectation_string could unpack into TestExpectationLine directly.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:260
> +            (expectation, errors) = TestExpectationParser.parse(line)

Note: You don't need the ( or ) when making a destructuring assignment, which you might view as prettier.
Comment 6 Dirk Pranke 2011-07-12 17:19:26 PDT
Comment on attachment 100579 [details]
Much better.

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

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:152
> +                    result.append(expectation.comment)

See comment, below.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:171
> +        [[<modifiers>] : <name> = <expectations>][ //<comment>]

Historically, comments are  only allowed on a line by themselves. Do you mean to change this? I don't know that I see a real advantage to supporting it, but I don't feel strongly about it either way.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:173
> +        Any errant whitespace is not preserved.

Do you wish to preserve blank lines? If you don't, round-tripping the file might make the file a lot harder for people to read.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208
>> +            comment = line[comment_index + 2:]
> 
> You can't split on substrings in python?

line, comment_part = [x.strip() for x in line.split('//')] ?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:247
> +class TestExpectationsFile:

I wouldn't use the name "File" here, if it's not actually pointing to a file (and it doesn't have to point to a file). Maybe Sequence or List?

Also, we used to have a TestExpectationsFile class that was basically the whole giant thing; calling it this might confuse the old-timers used to the earlier code.
Comment 7 Dimitri Glazkov (Google) 2011-07-12 20:40:35 PDT
Comment on attachment 100579 [details]
Much better.

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

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:171
>> +        [[<modifiers>] : <name> = <expectations>][ //<comment>]
> 
> Historically, comments are  only allowed on a line by themselves. Do you mean to change this? I don't know that I see a real advantage to supporting it, but I don't feel strongly about it either way.

No, that was existing behavior. There are actually a couple of uses of this syntax in test_expectations today. I was really tempted to just get rid of them, but decided against it to minimize change.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:173
>> +        Any errant whitespace is not preserved.
> 
> Do you wish to preserve blank lines? If you don't, round-tripping the file might make the file a lot harder for people to read.

Blank lines are preserved. In fact, line = TestExpectationLine() produces a blank line :)

The errant whitespace is the extra spacing around modifiers, name, expectations, and comment (see the test_string_whitespace_stripping unit test for examples)

>>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208
>>> +            comment = line[comment_index + 2:]
>> 
>> You can't split on substrings in python?
> 
> line, comment_part = [x.strip() for x in line.split('//')] ?

D'oi! :) I'll fix that up. Thanks, Python gurus!

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:217
>> +            return (None, None, None, None)
> 
> Consider using a dictionary instead of a tuple.  That's more self-documenting / self-error-correcting.

Yeah, I'll rework this.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:233
>> +class TestExpectationLine:
> 
> Seems like _split_expectation_string could unpack into TestExpectationLine directly.

Yep.

>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:247
>> +class TestExpectationsFile:
> 
> I wouldn't use the name "File" here, if it's not actually pointing to a file (and it doesn't have to point to a file). Maybe Sequence or List?
> 
> Also, we used to have a TestExpectationsFile class that was basically the whole giant thing; calling it this might confuse the old-timers used to the earlier code.

TestExpectationsList sounds good.
Comment 8 Dimitri Glazkov (Google) 2011-07-13 08:39:18 PDT
Comment on attachment 100579 [details]
Much better.

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

>>>> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:208
>>>> +            comment = line[comment_index + 2:]
>>> 
>>> You can't split on substrings in python?
>> 
>> line, comment_part = [x.strip() for x in line.split('//')] ?
> 
> D'oi! :) I'll fix that up. Thanks, Python gurus!

Ah, turns out it's not that easy. We should allow for a comment consisting of eight slashes, for example.
Comment 9 Dimitri Glazkov (Google) 2011-07-13 08:42:04 PDT
I am gonna land this and polish with follow-up patches.
Comment 10 Dimitri Glazkov (Google) 2011-07-13 08:49:17 PDT
Committed r90920: <http://trac.webkit.org/changeset/90920>