Bug 122154 – [Tools] User interruption (Ctrl+C) on run-webkit-tests should also generate results.html

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Ravi Kasibhatla]

Currently, results.html is generated from the results json file only on completion of entire layout tests run. Ideally, it should be created even when the execution has been interrupted - either by user (by Ctrl+C) or because of other interruption (exit-after-n-failures option). In both cases, all failures should be reported.

We need not throw the results.html onto the browser window in case of interruption, to avoid creation of browser session for all unwanted runs, but it should still dump the results into the html - just in case the user wants to go over them later (before next run).

This behavior change was reported in blink (https://code.google.com/p/chromium/issues/detail?id=263151) and got fixed through CL (http://src.chromium.org/viewvc/blink?view=rev&rev=157769).

Any thoughts on the integration of the changes in WebKit repo?

[Ravi Kasibhatla]

[Merging from Blink]

[Ravi Kasibhatla]

Created attachment 213073 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 213073 [details]
Patch

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.

[Ravi Kasibhatla]

Created attachment 213235 [details]
Patch

[Ravi Kasibhatla]

(In reply to comment #3)
> (From update of attachment 213073 [details])
> 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.
Done. Corrected.
> 
> > 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".
Done. Corrected.
> 
> > 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?
I needed INTERRUPTED_EXIT_STATUS value in manager.py. It is wrong (from design view) to include run_webkit_tests.py from manager.py, since we include manager.py inside run_webkit_tests.py - it creates a loop. Rather I felt it was better to include it in test_run_results.py and use it at all places.
> 
> > 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?
As I understand, the regular interrupt is generated when the testing is stopped in between due to no. of failures reaching a limit specified by exit-after-n-failures option. On build bots, it is important to throw the failure result if the execution stopped due to failures hitting the limit. Hence, not adding that condition. The build bot printer shouldn't result in case of unittesting or keyboard 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
Done. Corrected.
> 
> > 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. 
How can we test for results.html in this unit test case - because it is not generated. Before start of this test case, we change the run() func pointer to point to a dummy run() which just raises the KeyboardInterrupt. So, in the testcase, we don't even read the test expectations to generate results json file, so no results.html. The only way to test for results.html generation is to run at least 1 actual test and then check for the file presence.
> r- due to the lack of tests.
What other tests do you intend to see? I didn't add any new tests, because there are already tests to check the usecase of keyboard interrupt.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #3)
> > > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:83
> > > +    # We need to still handle KeyboardInterrupt, atleast for webkitpy unittest cases.
> > 
> > Type: at least
> Done. Corrected.
> > 
> > > 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. 
>
> How can we test for results.html in this unit test case - because it is not generated. Before start of this test case, we change the run() func pointer to point to a dummy run() which just raises the KeyboardInterrupt. So, in the testcase, we don't even read the test expectations to generate results json file, so no results.html. The only way to test for results.html generation is to run at least 1 actual test and then check for the file presence.

There are tests for generating results.html.  Please take a look at them and add a new test case.

> What other tests do you intend to see?

I don't think we need multiple test cases for this change.

[Csaba Osztrogonác]

Comment on attachment 213235 [details]
Patch

I can't see how to test easily sending a ctrl+C during testing.

It patch works fine for me and in Blink too since 1.5 years ago.
We shouldn't waste our time for thinking about sophisticated testing
for this simple patch, let's land it and enjoy the results.

[Csaba Osztrogonác]

Committed r184382: <http://trac.webkit.org/changeset/184382>