[webkit-reviews] review denied: [Bug 42832] webkit-patch command to find the ports covering a specific layout test : [Attachment 62405] proposed patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 42832: webkit-patch command to find the ports covering a specific layout
test
https://bugs.webkit.org/show_bug.cgi?id=42832

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
WebKitTools/Scripts/webkitpy/common/checkout/scm.py:244
 +	def directory_contents(self, path):
Please add unit tests in scm_unittests.  To run the SCM unit tests, you need to
pass --all to test-webkitpy.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:301
 +			     if line.strip() and not line.startswith("#")]
This calls line.strip twice.  :(

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:307
 +			tests_skipped.append(test_name)
skipped_tests and tests_skipped are very similar names.  It's hard to know
which is which.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:297
 +	def _find_tests_in_skipped(self, scm, tests, skipped_file):
We shouldn't be parsing Skipped files at the command layer.  That's something
much lower level.  We need to add some facilities in one of the lower-level
modules so that we can re-use this parsing code in other commands.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:319
 +	    chromium_expectations_path = os.path.join(platform_directory,
"chromium", "test_expectations.txt")
The command layer shouldn't know anything about specific ports.  All that
information needs to be encapsulated in the port abstraction.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:321
 +		     if line.find("SKIP") >= 0 and line.strip() and not
line.startswith("//")]
We already have a very nice facility for parsing test_expectations.  Please
don't re-invent the wheel.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:350
 +	    scm = tool.scm()
We don't generally store scm in a local variable.  We just access it off the
tool.

WebKitTools/Scripts/webkitpy/tool/commands/queries.py:351
 +	    platform_directory =
tool.scm().absolute_path(os.path.join("LayoutTests", "platform"))
This knowledge doesn't belong at the command layer.

WebKitTools/Scripts/webkitpy/tool/mocktool.py:438
 +  # I am a Skipped file
This sort of fixture belongs in the unit test for a module that understands
Skipped files.	These fixtures are a bit higher level.	That's just a symptom
of trying to put too much at the command layer.