[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
bugzilla-daemon at webkit.org
Tue Oct 1 11:36:09 PDT 2013
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.
More information about the webkit-reviews
mailing list