[webkit-reviews] review denied: [Bug 122154] [Tools] User interruption (Ctrl+C) on run-webkit-tests should also generate results.html : [Attachment 213073] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Ravi Kasibhatla
<r.kasibhatla at samsung.com>'s request for review:
Bug 122154: [Tools] User interruption (Ctrl+C) on run-webkit-tests should also
generate results.html
https://bugs.webkit.org/show_bug.cgi?id=122154

Attachment 213073: Patch
https://bugs.webkit.org/attachment.cgi?id=213073&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=213073&action=review


> Tools/ChangeLog:4
> +	   User interruption (Ctrl+C) on run-webkit-tests should also generate
> +	   results.html

It doesn't seem like a good idea to wrap the line right before the last word.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:248
> +		   if self._options.show_results and
(initial_results.unexpected_results_by_name or
> +						     
(self._options.full_results_html and initial_results.total_failures)):

Wrong indentation. In WebKit, we use 4 space indentation everywhere so
"(self._options" should appear exactly 4 spaces on the right of "if
self._options.show_results".

> Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:39
> +INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128

Why was this moved from run_webkit_tests.py?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:78
> +	   if run_details.exit_code != -1 and not
run_details.initial_results.keyboard_interrupted:

How about the regular interrupt?

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:83
> +    # We need to still handle KeyboardInterrupt, atleast for webkitpy
unittest cases.

Type: at least

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:960
> -	       self.assertEqual(res, run_webkit_tests.INTERRUPTED_EXIT_STATUS)
> +	       self.assertEqual(res, INTERRUPTED_EXIT_STATUS)

We need an assertion to make sure results.html is generated. r- due to the lack
of tests.