Bug 64386 – Extract model-like TestExpectationLine and TestExpectationFile from TestExpectations.

• Component: New Bugs
• Status: RESOLVED FIXED

[Dimitri Glazkov (Google)]

Extract model-like TestExpectationLine and TestExpectationFile from TestExpectations.

[Dimitri Glazkov (Google)]

Created attachment 100553 [details]
WIP: Needs tests.

[Dimitri Glazkov (Google)]

Created attachment 100575 [details]
With tests.

[Dimitri Glazkov (Google)]

Comment on attachment 100575 [details]
With tests.

this is wrong. fixing.

[Dimitri Glazkov (Google)]

Created attachment 100579 [details]
Much better.

[Adam Barth]

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.

[Dirk Pranke]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

I am gonna land this and polish with follow-up patches.

[Dimitri Glazkov (Google)]

Committed r90920: <http://trac.webkit.org/changeset/90920>