Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23496003:
User interruption should still create results.html for run-webkit-tests. (Closed)
|
Created:
7 years, 3 months ago by r.kasibhatla Modified:
7 years, 3 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionUser interruption should still create results.html for run-webkit-tests.
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 or because of
any interruption. In both cases, all failures should be reported.
BUG=263151
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157769
Patch Set 1 #
Total comments: 3
Patch Set 2 : Review comments incorporated #
Total comments: 3
Patch Set 3 : Rework-patchset3 #
Total comments: 1
Patch Set 4 : Fixed failing webkitpy tests and skipping printing of bot results in case of user interruption. #
Total comments: 2
Patch Set 5 : #
Total comments: 14
Patch Set 6 : Corrected INTERRUPTED_EXIT_STATUS import #
Messages
Total messages: 33 (0 generated)
https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... File LayoutTests/fast/harness/results.html (left): https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... LayoutTests/fast/harness/results.html:1299: html += "<p class='stopped-running-early-message'>Testing exited early.</p>" Why would you want to remove this? It seems we should still notify the user that they're working with a partial results run, no?
https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... File LayoutTests/fast/harness/results.html (left): https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... LayoutTests/fast/harness/results.html:1299: html += "<p class='stopped-running-early-message'>Testing exited early.</p>" On 2013/08/27 14:55:08, eseidel - vacation until 9.3 wrote: > Why would you want to remove this? > > It seems we should still notify the user that they're working with a partial > results run, no? I agree w/ eseidel. This should stay. https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: raise If your intent is to create and show the results.json even on a ctrl-C, you'll need to change this "raise" to something else, I think. Also, there should probably be some tests in run_webkit_tests_integration.py that need to be updated (or at least should be updated) to capture the new intent.
On 2013/08/27 18:18:24, Dirk Pranke wrote: > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > File LayoutTests/fast/harness/results.html (left): > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > LayoutTests/fast/harness/results.html:1299: html += "<p > class='stopped-running-early-message'>Testing exited early.</p>" > On 2013/08/27 14:55:08, eseidel - vacation until 9.3 wrote: > > Why would you want to remove this? > > > > It seems we should still notify the user that they're working with a partial > > results run, no? > > I agree w/ eseidel. This should stay. > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > (right): > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: raise > If your intent is to create and show the results.json even on a ctrl-C, you'll > need to change this "raise" to something else, I think. @dpranke: I didn't get why do we need to remove "raise"? As I understand, this is to push up the exception higher on the stack to run_webkit_tests.py, based on which it returns the different return value - INTERRUPTED_EXIT_STATUS. As for results.json, with current patch it is still getting generated - because we are returning run_results from layout_test_runner.py run_tests() function. Am I wrong in keeping both raise and return statements in run_tests. > > Also, there should probably be some tests in run_webkit_tests_integration.py > that need to be updated (or at least should be updated) to capture the new > intent. I am working to update the patch with tests and correcting the html.
On 2013/08/28 06:31:34, r.kasibhatla wrote: > On 2013/08/27 18:18:24, Dirk Pranke wrote: > > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > > File LayoutTests/fast/harness/results.html (left): > > > > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > > LayoutTests/fast/harness/results.html:1299: html += "<p > > class='stopped-running-early-message'>Testing exited early.</p>" > > On 2013/08/27 14:55:08, eseidel - vacation until 9.3 wrote: > > > Why would you want to remove this? > > > > > > It seems we should still notify the user that they're working with a partial > > > results run, no? > > > > I agree w/ eseidel. This should stay. > > > > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > > (right): > > > > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: > raise > > If your intent is to create and show the results.json even on a ctrl-C, you'll > > need to change this "raise" to something else, I think. > @dpranke: I didn't get why do we need to remove "raise"? As I understand, this > is to push up the exception higher on the stack to run_webkit_tests.py, based on > which it returns the different return value - INTERRUPTED_EXIT_STATUS. > As for results.json, with current patch it is still getting generated - because > we are returning run_results from layout_test_runner.py run_tests() function. Am > I wrong in keeping both raise and return statements in run_tests. What I mean is that if you are running the tests and hit ctrl-C, the KeyboardInterrupt is raised, and we jump out of run_tests() all the way up to main() in run_webkit_tests.py . That means that the run_results is not returned, we don't write results.json, or have a chance to show the results.html . If you want any of those things to still happen, you can't raise the exception here, you'll have to change the code. If you don't, then you don't need to change anything, but perhaps I misunderstood the intent of your change?
On 2013/08/28 18:38:28, Dirk Pranke wrote: > On 2013/08/28 06:31:34, r.kasibhatla wrote: > > On 2013/08/27 18:18:24, Dirk Pranke wrote: > > > > > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > > > File LayoutTests/fast/harness/results.html (left): > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > > > LayoutTests/fast/harness/results.html:1299: html += "<p > > > class='stopped-running-early-message'>Testing exited early.</p>" > > > On 2013/08/27 14:55:08, eseidel - vacation until 9.3 wrote: > > > > Why would you want to remove this? > > > > > > > > It seems we should still notify the user that they're working with a > partial > > > > results run, no? > > > > > > I agree w/ eseidel. This should stay. > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > > > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > > > (right): > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: > > raise > > > If your intent is to create and show the results.json even on a ctrl-C, > you'll > > > need to change this "raise" to something else, I think. > > @dpranke: I didn't get why do we need to remove "raise"? As I understand, this > > is to push up the exception higher on the stack to run_webkit_tests.py, based > on > > which it returns the different return value - INTERRUPTED_EXIT_STATUS. > > As for results.json, with current patch it is still getting generated - > because > > we are returning run_results from layout_test_runner.py run_tests() function. > Am > > I wrong in keeping both raise and return statements in run_tests. > > What I mean is that if you are running the tests and hit ctrl-C, the > KeyboardInterrupt is raised, > and we jump out of run_tests() all the way up to main() in run_webkit_tests.py . > That means > that the run_results is not returned, we don't write results.json, or have a > chance to show the > results.html . > > If you want any of those things to still happen, you can't raise the exception > here, you'll have to > change the code. If you don't, then you don't need to change anything, but > perhaps I misunderstood > the intent of your change? #dpranke: In my current patch, I have changed the logic even in case of KeyboardInterrupt. In the finally block following the "exception KeyboardInterrupt:", I am actually returning the run_results, which will allow me to create {full|failing}_results.json and the entire result generation is working properly. But I agree, it might be incorrect to raise an exception and then doing return as well, and I am looking to correct it in the coming patch.
On 2013/08/29 05:48:59, r.kasibhatla wrote: > On 2013/08/28 18:38:28, Dirk Pranke wrote: > > On 2013/08/28 06:31:34, r.kasibhatla wrote: > > > On 2013/08/27 18:18:24, Dirk Pranke wrote: > > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > > > > File LayoutTests/fast/harness/results.html (left): > > > > > > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/LayoutTests/fast/harness/resu... > > > > LayoutTests/fast/harness/results.html:1299: html += "<p > > > > class='stopped-running-early-message'>Testing exited early.</p>" > > > > On 2013/08/27 14:55:08, eseidel - vacation until 9.3 wrote: > > > > > Why would you want to remove this? > > > > > > > > > > It seems we should still notify the user that they're working with a > > partial > > > > > results run, no? > > > > > > > > I agree w/ eseidel. This should stay. > > > > > > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > > > > File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/23496003/diff/1/Tools/Scripts/webkitpy/layout... > > > > Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: > > > raise > > > > If your intent is to create and show the results.json even on a ctrl-C, > > you'll > > > > need to change this "raise" to something else, I think. > > > @dpranke: I didn't get why do we need to remove "raise"? As I understand, > this > > > is to push up the exception higher on the stack to run_webkit_tests.py, > based > > on > > > which it returns the different return value - INTERRUPTED_EXIT_STATUS. > > > As for results.json, with current patch it is still getting generated - > > because > > > we are returning run_results from layout_test_runner.py run_tests() > function. > > Am > > > I wrong in keeping both raise and return statements in run_tests. > > > > What I mean is that if you are running the tests and hit ctrl-C, the > > KeyboardInterrupt is raised, > > and we jump out of run_tests() all the way up to main() in run_webkit_tests.py > . > > That means > > that the run_results is not returned, we don't write results.json, or have a > > chance to show the > > results.html . > > > > If you want any of those things to still happen, you can't raise the exception > > here, you'll have to > > change the code. If you don't, then you don't need to change anything, but > > perhaps I misunderstood > > the intent of your change? > > #dpranke: In my current patch, I have changed the logic even in case of > KeyboardInterrupt. In the finally block following the "exception > KeyboardInterrupt:", I am actually returning the run_results, which will allow > me to create {full|failing}_results.json and the entire result generation is > working properly. > But I agree, it might be incorrect to raise an exception and then doing return > as well, and I am looking to correct it in the coming patch. I was trying to correct run_webkit_tests_integrationtest.py to reflect the new functionality for keyboard interrupt and replace the assertRaises with assertEqual with exit code of INTERRUPTED_EXIT_STATUS. It gives exception while doing git cl upload that RunDetails doesn't has the member assertEqual. Unlike the other regular python scripts, I am unable to figure out when this script is invoked and how these RunTest class functions are invoked. For reference on the type of changes I intend to make to run_webkit_tests_integrationtest.py, I have added the changes in comments in the newly upload patch. Can somebody help me out here?
I think we're getting closer, just a few minor comments. As far as the presubmit warning goes, I've seen that a couple times myself, and it's a bug in pylint (this is getting pulled in via PRESUBMIT.py and depot_tools). It's usually safe to ignore this, so just upload it with --bypass-hooks and add a comment about the warning to the emails so the reviewer can double-check. https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:259: self._printer.print_results(time.time() - start_time, initial_results, summarized_failing_results) I think we probably shouldn't print the results if the run was manually interrupted, but I'm not 100% certain of this. Context: the basic idea behind the way I implemented the keyboard-interrupt support was to exit as quickly as possible while still exiting cleanly and to do what amount of work still seemed to be sensible (w/o blocking). So, I didn't create the results.html file or print things because it was slow and because it might not make sense. I think creating the results probably isn't actually that slow, and I can see the argument that it makes sense in some or maybe even most cases. However, I doubt printing the results make sense (even if it's not slow). https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:49: INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128 You can delete this line (and the 'import signal, above') and move the comment over into manager.py. https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (right): https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:296: #self.assertEqual(details.exit_code, run_webkit_tests.INTERRUPTED_EXIT_STATUS) I'm not quite sure what your intent w/ these changes are; better to just delete the lines of code and show me what ends up happening.
On 2013/08/29 17:04:26, Dirk Pranke wrote: > I think we're getting closer, just a few minor comments. > > As far as the presubmit warning goes, I've seen that a couple times myself, and > it's a bug in pylint (this is getting pulled in via PRESUBMIT.py and > depot_tools). > > It's usually safe to ignore this, so just upload it with --bypass-hooks and add > a comment about the warning to the emails so the reviewer can double-check. > > https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... > File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): > > https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:259: > self._printer.print_results(time.time() - start_time, initial_results, > summarized_failing_results) > I think we probably shouldn't print the results if the run was manually > interrupted, but I'm not 100% certain of this. > > Context: the basic idea behind the way I implemented the keyboard-interrupt > support was to exit as quickly as possible while still exiting cleanly and to do > what amount of work still seemed to be sensible (w/o blocking). So, I didn't > create the results.html file or print things because it was slow and because it > might not make sense. I think creating the results probably isn't actually that > slow, and I can see the argument that it makes sense in some or maybe even most > cases. However, I doubt printing the results make sense (even if it's not slow). > > https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... > File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): > > https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:49: > INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128 > You can delete this line (and the 'import signal, above') and move the comment > over into manager.py. > > https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... > File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py > (right): > > https://codereview.chromium.org/23496003/diff/9001/Tools/Scripts/webkitpy/lay... > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:296: > #self.assertEqual(details.exit_code, run_webkit_tests.INTERRUPTED_EXIT_STATUS) > I'm not quite sure what your intent w/ these changes are; better to just delete > the lines of code and show me what ends up happening. In run_webkit_tests_integrationtest.py, the unit test case result was being verified using assertEqual with parameter being exit code, for all the success results. Since, now on keyboard interrupt we are returning exit code with results, I intended to change the test case result accordingly and verify the exit code. By deleting the lines of code to see whats happenings, are you suggesting to revert my changes and keep the unit test case as originally defined before my change? Working on your other comments.
On Thu, Aug 29, 2013 at 12:28 PM, <r.kasibhatla@samsung.com> wrote: > In run_webkit_tests_**integrationtest.py, the unit test case result was > being > verified using assertEqual with parameter being exit code, for all the > success > results. Since, now on keyboard interrupt we are returning exit code with > results, I intended to change the test case result accordingly and verify > the > exit code. By deleting the lines of code to see whats happenings, are you > suggesting to revert my changes and keep the unit test case as originally > defined before my change? > No, I meant literally delete the lines you have commented out so I can better see how you expect the test to run now. -- Dirk To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On 2013/08/29 19:31:36, Dirk Pranke wrote: > On Thu, Aug 29, 2013 at 12:28 PM, <mailto:r.kasibhatla@samsung.com> wrote: > > > In run_webkit_tests_**integrationtest.py, the unit test case result was > > being > > verified using assertEqual with parameter being exit code, for all the > > success > > results. Since, now on keyboard interrupt we are returning exit code with > > results, I intended to change the test case result accordingly and verify > > the > > exit code. By deleting the lines of code to see whats happenings, are you > > suggesting to revert my changes and keep the unit test case as originally > > defined before my change? > > > > No, I meant literally delete the lines you have commented out so I can > better see how you expect the test to run now. > > -- Dirk > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org. Finished other review comments. For run_webkit_tests_integrationtest.py, the commented lines were my actual changes and how I intended to run the test now. I just put them in comments since presubmit check was not allowing me to upload with the changes. I have now enabled my changes and uploaded using --bypass-hooks.
lgtm. https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:261: self._printer.print_results(time.time() - start_time, initial_results, summarized_failing_results) I see that you've changed this so that printer.print_results() is only called if we weren't interrupted (as a result of my prior comment). I think this will be fine. However, we also print out results to stdout for the buildbot to parse in run_webkit_tests.py:77-79 , and so we'll still execute that on a ctrl-C, I think. The catch here is that the run_webkit_tests.py lines *have* to be printed on the bots if we exit early because of too many errors, but locally, we probably don't want this printed out if we do a ctrl-C. That would require you to be able to distinguish the two cases. I'm not sure if we really need to fix this or not; mostly it depends on how annoying the extra logging will be. Can you take a quick look and see what you think (and whether it's relatively easy to fix)? If not, I'm okay w/ landing this patch as-is and revisiting separately if we need to fix that.
On 2013/09/03 20:55:27, Dirk Pranke wrote: > lgtm. > > https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... > File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): > > https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:261: > self._printer.print_results(time.time() - start_time, initial_results, > summarized_failing_results) > I see that you've changed this so that printer.print_results() is only called if > we weren't interrupted (as a result of my prior comment). > > I think this will be fine. However, we also print out results to stdout for the > buildbot to parse in run_webkit_tests.py:77-79 , and so we'll still execute that > on a ctrl-C, I think. > > The catch here is that the run_webkit_tests.py lines *have* to be printed on the > bots if we exit early because of too many errors, but locally, we probably don't > want this printed out if we do a ctrl-C. That would require you to be able to > distinguish the two cases. > > I'm not sure if we really need to fix this or not; mostly it depends on how > annoying the extra logging will be. Can you take a quick look and see what you > think (and whether it's relatively easy to fix)? If not, I'm okay w/ landing > this patch as-is and revisiting separately if we need to fix that. @dpranke: I will check for the possibility of separation. If that would be small change, I will add it to this patch. If its coming to bigger pat
On 2013/09/04 02:26:50, r.kasibhatla wrote: > On 2013/09/03 20:55:27, Dirk Pranke wrote: > > lgtm. > > > > > https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... > > File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): > > > > > https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... > > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:261: > > self._printer.print_results(time.time() - start_time, initial_results, > > summarized_failing_results) > > I see that you've changed this so that printer.print_results() is only called > if > > we weren't interrupted (as a result of my prior comment). > > > > I think this will be fine. However, we also print out results to stdout for > the > > buildbot to parse in run_webkit_tests.py:77-79 , and so we'll still execute > that > > on a ctrl-C, I think. > > > > The catch here is that the run_webkit_tests.py lines *have* to be printed on > the > > bots if we exit early because of too many errors, but locally, we probably > don't > > want this printed out if we do a ctrl-C. That would require you to be able to > > distinguish the two cases. > > > > I'm not sure if we really need to fix this or not; mostly it depends on how > > annoying the extra logging will be. Can you take a quick look and see what you > > think (and whether it's relatively easy to fix)? If not, I'm okay w/ landing > > this patch as-is and revisiting separately if we need to fix that. > > @dpranke: I will check for the possibility of separation. If that would be small > change, I will add it to this patch. If its coming to bigger pat Sorry, by mistake the "Send Message" got clicked. What I meant was, if the patch is slightly complicated, I will put up another patch for the distinguishing issue.
On 2013/09/04 02:28:22, r.kasibhatla wrote: > On 2013/09/04 02:26:50, r.kasibhatla wrote: > > On 2013/09/03 20:55:27, Dirk Pranke wrote: > > > lgtm. > > > > > > > > > https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... > > > File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): > > > > > > > > > https://codereview.chromium.org/23496003/diff/19001/Tools/Scripts/webkitpy/la... > > > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:261: > > > self._printer.print_results(time.time() - start_time, initial_results, > > > summarized_failing_results) > > > I see that you've changed this so that printer.print_results() is only > called > > if > > > we weren't interrupted (as a result of my prior comment). > > > > > > I think this will be fine. However, we also print out results to stdout for > > the > > > buildbot to parse in run_webkit_tests.py:77-79 , and so we'll still execute > > that > > > on a ctrl-C, I think. > > > > > > The catch here is that the run_webkit_tests.py lines *have* to be printed on > > the > > > bots if we exit early because of too many errors, but locally, we probably > > don't > > > want this printed out if we do a ctrl-C. That would require you to be able > to > > > distinguish the two cases. > > > > > > I'm not sure if we really need to fix this or not; mostly it depends on how > > > annoying the extra logging will be. Can you take a quick look and see what > you > > > think (and whether it's relatively easy to fix)? If not, I'm okay w/ landing > > > this patch as-is and revisiting separately if we need to fix that. > > > > @dpranke: I will check for the possibility of separation. If that would be > small > > change, I will add it to this patch. If its coming to bigger pat > > Sorry, by mistake the "Send Message" got clicked. What I meant was, if the patch > is slightly complicated, I will put up another patch for the distinguishing > issue. We can distinguish between the two usecases using the exit_code - let it be INTERRUPTED_EXIT_STATUS for Ctrl^C and num_regressions for the other. Then a simple check of if run_details.exit_code != -1 and run_details.exit_code != INTERRUPTED_EXIT_STATUS: would suffix. As we cannot distinguish based on TestRunResults.interrupted flag, we need to set the exit_code differently. One approach I could think of get rid of TestRunResults.interrupted and add exit_code to TestRunResults so I can set it correctly as per usecase. Should I take it up as separate patch?
> We can distinguish between the two usecases using the exit_code - let it be > INTERRUPTED_EXIT_STATUS for Ctrl^C and num_regressions for the other. Then a > simple check of if run_details.exit_code != -1 and run_details.exit_code != > INTERRUPTED_EXIT_STATUS: would suffix. > As we cannot distinguish based on TestRunResults.interrupted flag, we need to > set the exit_code differently. One approach I could think of get rid of > TestRunResults.interrupted and add exit_code to TestRunResults so I can set it > correctly as per usecase. > Should I take it up as separate patch? Friendly remainder.
On 2013/09/05 03:36:35, r.kasibhatla wrote: > > We can distinguish between the two usecases using the exit_code - let it be > > INTERRUPTED_EXIT_STATUS for Ctrl^C and num_regressions for the other. Then a > > simple check of if run_details.exit_code != -1 and run_details.exit_code != > > INTERRUPTED_EXIT_STATUS: would suffix. > > As we cannot distinguish based on TestRunResults.interrupted flag, we need to > > set the exit_code differently. One approach I could think of get rid of > > TestRunResults.interrupted and add exit_code to TestRunResults so I can set it > > correctly as per usecase. So, there's two minor problems with your suggestion. The first is that the exit status is kind of an inherently port-specific / platform-dependent concept, and so I'm a little leery of pushing this into the run_results. Though, maybe I'm just being overly paranoid here. Second, INTERRUPTED_EXIT_STATUS is actually 130. So, there is the nonzero chance that we'll get confused with a run with 130 regressions. A crufty alternative is to have a separate 'keyboard_interrupted' field in TestRunResults. We actually had this at one point in one form and was able to merge it together w/ interrupted, but maybe it needs to be un-merged. Ojan, what do you think of this patch and this problem?
On 2013/09/05 20:08:43, Dirk Pranke wrote: > On 2013/09/05 03:36:35, r.kasibhatla wrote: > > > We can distinguish between the two usecases using the exit_code - let it be > > > INTERRUPTED_EXIT_STATUS for Ctrl^C and num_regressions for the other. Then a > > > simple check of if run_details.exit_code != -1 and run_details.exit_code != > > > INTERRUPTED_EXIT_STATUS: would suffix. > > > As we cannot distinguish based on TestRunResults.interrupted flag, we need > to > > > set the exit_code differently. One approach I could think of get rid of > > > TestRunResults.interrupted and add exit_code to TestRunResults so I can set > it > > > correctly as per usecase. > > So, there's two minor problems with your suggestion. The first is that the exit > status is kind of an inherently port-specific / platform-dependent concept, and > so I'm a little leery of pushing this into the run_results. Though, maybe I'm > just being overly paranoid here. > > Second, INTERRUPTED_EXIT_STATUS is actually 130. So, there is the nonzero chance > that we'll get confused with a run with 130 regressions. > > A crufty alternative is to have a separate 'keyboard_interrupted' field in > TestRunResults. We actually had this at one point in one form and was able to > merge it together w/ interrupted, but maybe it needs to be un-merged. > > Ojan, what do you think of this patch and this problem? @Ojan: Can you please check the patch and comment?
I chatted w/ ojan a bit on this. We're fine w/ this patch as-is for now. If issues arise, we'll deal with them then. lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/23496003/19001
Retried try job too often on linux_blink_rel for step(s) webkit_python_tests, webkit_tests, webkit_unit_tests, weborigin_unittests, wtf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
On 2013/09/10 19:58:30, Dirk Pranke wrote: > I chatted w/ ojan a bit on this. We're fine w/ this patch as-is for now. If > issues arise, we'll deal with them then. > > lgtm. @dpranke, @ojan: PTAL. I have fixed the failing of webkitpy unit tests. Also, in order to fix the unit tests, I had to distinguish between the keyboard interruption and the interruption due to failure test count exceeding. So, I have addressed the earlier comment of *not* printing the bot results, in case of user interruption.
lgtm w/ changes to put the exit code in test_run_results.py , if that works for you. https://codereview.chromium.org/23496003/diff/47001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/23496003/diff/47001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:257: exit_code = signal.SIGINT + 128 Seeing as how this constant is actually needed across three files, we should have a single definition *somewhere*. Thinking about it now, maybe this should be a constant on TestRunResults, e.g. 'exit_code = initial_results.INTERRUPTED_EXIT_STATUS'. Then run_webkit_tests.py and run_webkit_tests_integrationtest.py are already importing that module, and everything's clean.
On 2013/09/12 21:29:06, Dirk Pranke wrote: > lgtm w/ changes to put the exit code in test_run_results.py , if that works for > you. > > https://codereview.chromium.org/23496003/diff/47001/Tools/Scripts/webkitpy/la... > File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): > > https://codereview.chromium.org/23496003/diff/47001/Tools/Scripts/webkitpy/la... > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:257: exit_code = > signal.SIGINT + 128 > Seeing as how this constant is actually needed across three files, we should > have a single definition *somewhere*. Thinking about it now, maybe this should > be a constant on TestRunResults, e.g. 'exit_code = > initial_results.INTERRUPTED_EXIT_STATUS'. Then run_webkit_tests.py and > run_webkit_tests_integrationtest.py are already importing that module, and > everything's clean. We cannot define it in TestRunResults. Though it would work in regular cases, but it would fail for unittests, since we *override* the run_webkit_results.run() with fake run_webkit_tests_integrationtest.MainTest.test_exception_handling.interrupted_run(). So, in unittest, we will have null run_details object and access to *initial_results* will fail. Alternatively, we can define it as a constant in test_run_results.py and use it in all 3 files (but in this case I would have to import test_run_results.py in run_webkit_tests.py and run_webkit_tests_integrationtest.py explicitly).
PTAL at new change. Not exactly how you wanted, but not sure we can do better than this. https://codereview.chromium.org/23496003/diff/47001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/23496003/diff/47001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:257: exit_code = signal.SIGINT + 128 On 2013/09/12 21:29:06, Dirk Pranke wrote: > Seeing as how this constant is actually needed across three files, we should > have a single definition *somewhere*. Thinking about it now, maybe this should > be a constant on TestRunResults, e.g. 'exit_code = > initial_results.INTERRUPTED_EXIT_STATUS'. Then run_webkit_tests.py and > run_webkit_tests_integrationtest.py are already importing that module, and > everything's clean. Uploaded new patch. As explained in previous comment, we cannot put it inside TestRunResults object because (atleast for unittests it would fail). I have added the INTERRUPTED_EXIT_STATUS constant (as a class level constant) in TestRunResults and since run_webkit_tests & run_webkit_tests_integrationtest are not importing test_run_results, I am importing TestRunResults class specifically now. Hope its fine with you. If yes, please stamp the change.
Okay, I apologize for all the back-and-forth on this. Thanks for bearing with me. I think we've got the final version in sight. This all lgtm with the changes below. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: # Mark execution got interrupted and return results for tests executed till now. I don't think we really need the comment here. The code is pretty clear. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:256: exit_code = test_run_results.TestRunResults.INTERRUPTED_EXIT_STATUS change this to test_run_results.INTERRUPTED_EXIT_STATUS (i.e., a module-level constant). https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:41: INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128 Okay, I think my memory was wrong before, because I was thinking this had an exit_code field, but that's RunDetails, below. Regardless, making it a class-level constant doesn't really help since you might need this even if you don't have a RunDetails object. Please make this a module-level constant instead. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:39: from webkitpy.layout_tests.models.test_run_results import TestRunResults Change this to from webkitpy.layout_tests.models.test_run_results import INTERRUPTED_EXIT_STATUS then that becomes a name in the module-level scope here as well, making it the same as if you weren't deleting lines 48-50 in the old patch. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:85: return TestRunResults.INTERRUPTED_EXIT_STATUS Now you can use the old unqualified name here. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:53: from webkitpy.layout_tests.models.test_run_results import TestRunResults and you won't need this any more. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:967: self.assertEqual(res, TestRunResults.INTERRUPTED_EXIT_STATUS) and you don't need this change, either.
PTAL. I made INTERRUPTED_EXIT_STATUS a module level constant for test_run_results and imported it directly in manager.py, run_webkit_tests.py and run_webkit_tests_integrationtest.py, so that we can call them directly as previous code. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/layout_test_runner.py:123: # Mark execution got interrupted and return results for tests executed till now. On 2013/09/13 05:37:31, Dirk Pranke wrote: > I don't think we really need the comment here. The code is pretty clear. Done. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:256: exit_code = test_run_results.TestRunResults.INTERRUPTED_EXIT_STATUS On 2013/09/13 05:37:31, Dirk Pranke wrote: > change this to test_run_results.INTERRUPTED_EXIT_STATUS (i.e., a module-level > constant). Done. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:41: INTERRUPTED_EXIT_STATUS = signal.SIGINT + 128 On 2013/09/13 05:37:31, Dirk Pranke wrote: > Okay, I think my memory was wrong before, because I was thinking this had an > exit_code field, but that's RunDetails, below. > > Regardless, making it a class-level constant doesn't really help since you might > need this even if you don't have a RunDetails object. > > Please make this a module-level constant instead. Done. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:39: from webkitpy.layout_tests.models.test_run_results import TestRunResults On 2013/09/13 05:37:31, Dirk Pranke wrote: > Change this to > > from webkitpy.layout_tests.models.test_run_results import > INTERRUPTED_EXIT_STATUS > > then that becomes a name in the module-level scope here as well, making it the > same as if you weren't deleting lines 48-50 in the old patch. Done. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:85: return TestRunResults.INTERRUPTED_EXIT_STATUS On 2013/09/13 05:37:31, Dirk Pranke wrote: > Now you can use the old unqualified name here. Done. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... File Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (right): https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:53: from webkitpy.layout_tests.models.test_run_results import TestRunResults On 2013/09/13 05:37:31, Dirk Pranke wrote: > and you won't need this any more. Done. https://codereview.chromium.org/23496003/diff/56001/Tools/Scripts/webkitpy/la... Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:967: self.assertEqual(res, TestRunResults.INTERRUPTED_EXIT_STATUS) On 2013/09/13 05:37:31, Dirk Pranke wrote: > and you don't need this change, either. Done.
close enough :). lgtm.
On 2013/09/13 16:00:26, Dirk Pranke wrote: > close enough :). lgtm. Thanks. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/r.kasibhatla@samsung.com/23496003/63001
Message was sent while issue was closed.
Change committed as 157769 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
