[webkit-reviews] review denied: [Bug 86690] Rename test_expectations.txt to TestExpectations : [Attachment 145254] Make tools aware of TestExpectations in addition to test_expectations.txt

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 86690: Rename test_expectations.txt to TestExpectations
https://bugs.webkit.org/show_bug.cgi?id=86690

Attachment 145254: Make tools aware of TestExpectations in addition to
test_expectations.txt
https://bugs.webkit.org/attachment.cgi?id=145254&action=review

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


The patch is generally fine and may be r+'able as-is (with a couple of nits
about reducing repetitive mentions of the path) but I'd like to understand the
answers to my questions before r+'ing it

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:-677
> -

Ick. I'm glad this wasn't being used :).

> Tools/Scripts/webkitpy/layout_tests/port/base.py:688
> +	       port_name = 'chromium'

Why are you pulling the logic out of webkit.py and chromium.py and pulling it
into this routine? If it's to merge the logic of Port and WebKitPort, I'd
prefer that to be addressed in a different patch dedicated to that just to
avoid confusion. 

Do you have a preference for avoiding overridden functions or something as well
that's leading to this change?

> Tools/Scripts/webkitpy/layout_tests/port/base.py:694
> +	   return self._filesystem.join(baseline_path, 'TestExpectations')

Why are you doing this rather than just renaming all of the files?

> Tools/Scripts/webkitpy/layout_tests/port/test.py:251
> +	   filesystem.write_text_file(LAYOUT_TEST_DIR +
'/platform/test/TestExpectations', """

Can you pull '/platform/test/TestExpectations' into a constant somewhere so
that it is shared between this and line 343, below?

> Tools/Scripts/webkitpy/layout_tests/port/webkit_unittest.py:97
> +	   self.assertEqual(port.path_to_test_expectations_file(),
'/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations')

We should pull
'/mock-checkout/LayoutTests/platform/testwebkitport/TestExpectations' into a
constant as well so it can be re-used across multiple tests.

> Tools/Scripts/webkitpy/style/checker.py:527
> +	   elif basename == 'test_expectations.txt' or basename ==
'TestExpectations':

Here's another place where I don't know that we need to support both syntaxes
simultaneously.

> Tools/Scripts/webkitpy/style/checkers/test_expectations.py:64
> +	   self._output_regex =
re.compile('.*(TestExpectations|test_expectations.txt):(?P<line>\d+)\s*(?P<mess
age>.+)')

ditto.

> Tools/Scripts/webkitpy/tool/steps/commit.py:59
> +	       if filename.endswith('test_expectations.txt') or
filename.endswith('TestExpectations'):

ditto.

> Tools/TestResultServer/static-dashboards/dashboard_base.js:546
> +var LEGACY_CHROMIUM_EXPECTATIONS_URL =
'http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/chromium/te
st_expectations.txt';

Do we have to support both URLs for a reason other than the reason you want to
support both filenames generally?