[webkit-reviews] review denied: [Bug 96569] implement first part of support for the new TestExpectations syntax : [Attachment 163745] clean up for review

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 96569: implement first part of support for the new TestExpectations syntax
https://bugs.webkit.org/show_bug.cgi?id=96569

Attachment 163745: clean up for review
https://bugs.webkit.org/attachment.cgi?id=163745&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=163745&action=review


R- just for the Failure issue.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:270
> +	   'Failure': 'TEXT',

Doesn't this need to be something like ['TEXT', 'IMAGE+TEXT', 'AUDIO']? So that
any of those happening won't be an unexpected failure?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:342
> +		   else:

Maybe make this elif state == 'name_found' so you can log an error if the state
is something else?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:350
> +		   else:

Ditto: elif state == 'expectations'  and log an error otherwise?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:378
> +	   if not expectations:
> +	       if 'SKIP' not in modifiers and 'REBASELINE' not in modifiers and
'SLOW' not in modifiers:
> +		   modifiers.append('SKIP')
> +	       expectations = ['PASS']

Should this have a FIXME to be removed when we stop supporting Skipped files?

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:380
> +	   expectation_line.modifiers = bugs + modifiers

Maybe have a FIXME to store bugs separately on the expectation_line?