[webkit-reviews] review granted: [Bug 47647] commit-queue should not fail patches due to flaky tests : [Attachment 70759] Fix typo in CommitQueue.layout_test_results (untestable)

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 47647: commit-queue should not fail patches due to flaky tests
https://bugs.webkit.org/show_bug.cgi?id=47647

Attachment 70759: Fix typo in CommitQueue.layout_test_results (untestable)
https://bugs.webkit.org/attachment.cgi?id=70759&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=70759&action=review

> WebKitTools/Scripts/webkitpy/common/config/ports.py:108
> +    @classmethod
> +    def layout_tests_results_path(cls):
> +	   return "/tmp/layout-test-results/results.html"

We should really change ports to be instances.	I think things should be well
enough isolated to make that work now.

> WebKitTools/Scripts/webkitpy/common/net/buildbot.py:220
> +	   # FIXME: We need to move this sort of 404 logic into
NetworkTransaction or similar.

Or into a network abstraction.

> WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask.py:30
> +from webkitpy.common.net.layouttestresults import LayoutTestResults

This import looks to be unused.

> WebKitTools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:60
> +    def refetch_patch(self, patch):
> +	   return patch

Nice.

> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:278
> +	   try:
> +	       # FIXME: We need a nice open() abstraction for better mocking
here.
> +	       with codecs.open(results_path, "r", "utf-8") as results_file:
> +		   return LayoutTestResults.results_from_string(results_file)
> +	   except OSError, e:  # File does not exist or can't be read.
> +	       return None

This whole idiom should be on tool.filesystem().read(path)

> WebKitTools/Scripts/webkitpy/tool/steps/steps_unittest.py:60
> -    def test_runtests_leopard_commit_queue_hack(self):
> +    def test_runtests_leopard_commit_queue_hack_step(self):

We have this problem of two tests with the same name a lot.  I wonder if we can
scan the codebase for them somehow.