[webkit-reviews] review denied: [Bug 69750] NRWT should handle duplicated expectations : [Attachment 116659] proposed fix

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Kristóf Kosztyó
<kkristof at inf.u-szeged.hu>'s request for review:
Bug 69750: NRWT should handle duplicated expectations
https://bugs.webkit.org/show_bug.cgi?id=69750

Attachment 116659: proposed fix
https://bugs.webkit.org/attachment.cgi?id=116659&action=review

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116659&action=review


> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:837
> +			       (test.name, index))

This needs to add an error to a list of errors, not call _log.error(); that way
it can be included in the total list of errors during the call to
_report_errors. That's why a ParseError() isn't getting raised. 

Unfortunately, the refactoring of this class has made adding an error harder
than it used to be, as there isn't a global list-of-errors object. maybe create
a self.skipped_tests_errors, and check that in self._report_errors()?

Also, if these are truly errors (and not warnings), you should probably not be
adding them to the model.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:263
> +	   self._exp = TestExpectations(port, 'failures/expected/text.html\n',
'BUGX : failures/expected/text.html = text\n', None)

This should be raising a ParseError; see comments above.