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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122154





--- Comment #5 from Ravi Kasibhatla <r.kasibhatla at samsung.com>  2013-10-03 00:56:23 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.