Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1709173002:
mojo: Delay ApplicationTestBase shell disconnection. (Closed)
|
Created:
4 years, 10 months ago by xhwang Modified:
4 years, 10 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmojo: Delay ApplicationTestBase shell disconnection.
BUG=587523
TEST=Reenable tests.
Committed: https://crrev.com/74cb68fa1c313c885c6022eea11ef6855d9ca0da
Cr-Commit-Position: refs/heads/master@{#377068}
Patch Set 1 : initial approach #
Total comments: 3
Patch Set 2 : updated #Patch Set 3 : rebase only #
Messages
Total messages: 25 (6 generated)
xhwang@chromium.org changed reviewers: + msw@chromium.org, rockot@chromium.org
I have no idea of what I am doing :) But this can be a good starting point for discussion. See the bug for more details.
See my note at http://crbug.com/587523#c24 I don't think this is correct, but I may be wrong.
Patchset #2 (id:20001) has been deleted
Updated based on the discussion in the bug. PTAL again!
rebase only
lgtm, i guess https://chromiumcodereview.appspot.com/1709173002/diff/1/mojo/shell/public/cp... File mojo/shell/public/cpp/lib/application_test_base.cc (right): https://chromiumcodereview.appspot.com/1709173002/diff/1/mojo/shell/public/cp... mojo/shell/public/cpp/lib/application_test_base.cc:165: Environment::DestroyDefaultRunLoop(); Does the order of closing the connection and destroying the run loop matter?
https://chromiumcodereview.appspot.com/1709173002/diff/1/mojo/shell/public/cp... File mojo/shell/public/cpp/lib/application_test_base.cc (right): https://chromiumcodereview.appspot.com/1709173002/diff/1/mojo/shell/public/cp... mojo/shell/public/cpp/lib/application_test_base.cc:165: Environment::DestroyDefaultRunLoop(); On 2016/02/23 18:02:42, msw wrote: > Does the order of closing the connection and destroying the run loop matter? Sorry, ignore this old comment.
https://chromiumcodereview.appspot.com/1709173002/diff/1/mojo/shell/public/cp... File mojo/shell/public/cpp/lib/application_test_base.cc (right): https://chromiumcodereview.appspot.com/1709173002/diff/1/mojo/shell/public/cp... mojo/shell/public/cpp/lib/application_test_base.cc:165: Environment::DestroyDefaultRunLoop(); On 2016/02/23 18:03:44, msw wrote: > On 2016/02/23 18:02:42, msw wrote: > > Does the order of closing the connection and destroying the run loop matter? > > Sorry, ignore this old comment. Actually it's a good question, and I don't know enough to answer that. It doesn't matter here since I am not changing the order, but it'd be interesting to know :)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709173002/60001
rockot: I need your OWNERS approval as well. Thanks!
rs lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by xhwang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1709173002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1709173002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== mojo: Delay ApplicationTestBase shell disconnection. BUG=587523 TEST=Reenable tests. ========== to ========== mojo: Delay ApplicationTestBase shell disconnection. BUG=587523 TEST=Reenable tests. Committed: https://crrev.com/74cb68fa1c313c885c6022eea11ef6855d9ca0da Cr-Commit-Position: refs/heads/master@{#377068} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/74cb68fa1c313c885c6022eea11ef6855d9ca0da Cr-Commit-Position: refs/heads/master@{#377068}
Message was sent while issue was closed.
This causes numerous mojo:mus_apptests to fail on Windows: https://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Windows... [ FAILED ] Command (in 103 ms): E:\b\build\slave\chromium_mojo\build\src\out\Release\mojo_runner.exe --use-temporary-user-data-dir --use-x11-test-config --override-use-gl-with-osmesa-for-tests --gtest_filter=WindowServerTest.RootWindow mojo:mus_apptests ------------------------------------------------------------------------ Note: Google Test filter = WindowServerTest.RootWindow [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from WindowServerTest [ RUN ] WindowServerTest.RootWindow [ERROR:child_process_base.cc(215)] Connection error to the shell. [ERROR:child_process_base.cc(215)] Connection error to the shell. ------------------------------------------------------------------------ Please take a look or revert soon.
Message was sent while issue was closed.
On 2016/02/23 22:32:37, msw wrote: > This causes numerous mojo:mus_apptests to fail on Windows: > https://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Windows... > > [ FAILED ] Command (in 103 ms): > E:\b\build\slave\chromium_mojo\build\src\out\Release\mojo_runner.exe > --use-temporary-user-data-dir --use-x11-test-config > --override-use-gl-with-osmesa-for-tests > --gtest_filter=WindowServerTest.RootWindow mojo:mus_apptests > ------------------------------------------------------------------------ > Note: Google Test filter = WindowServerTest.RootWindow > [==========] Running 1 test from 1 test case. > [----------] Global test environment set-up. > [----------] 1 test from WindowServerTest > [ RUN ] WindowServerTest.RootWindow > [ERROR:child_process_base.cc(215)] Connection error to the shell. > [ERROR:child_process_base.cc(215)] Connection error to the shell. > > ------------------------------------------------------------------------ > > Please take a look or revert soon. Hmm, why this isn't caught by any bots? It seems this is windows only. I don't have a recent Windows check out to try this. Can anyone with a Windows checkout help on this? Otherwise I'll revert my CL.
Message was sent while issue was closed.
On 2016/02/23 22:36:58, xhwang wrote: > On 2016/02/23 22:32:37, msw wrote: > > This causes numerous mojo:mus_apptests to fail on Windows: > > > https://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Windows... > > > > [ FAILED ] Command (in 103 ms): > > E:\b\build\slave\chromium_mojo\build\src\out\Release\mojo_runner.exe > > --use-temporary-user-data-dir --use-x11-test-config > > --override-use-gl-with-osmesa-for-tests > > --gtest_filter=WindowServerTest.RootWindow mojo:mus_apptests > > ------------------------------------------------------------------------ > > Note: Google Test filter = WindowServerTest.RootWindow > > [==========] Running 1 test from 1 test case. > > [----------] Global test environment set-up. > > [----------] 1 test from WindowServerTest > > [ RUN ] WindowServerTest.RootWindow > > [ERROR:child_process_base.cc(215)] Connection error to the shell. > > [ERROR:child_process_base.cc(215)] Connection error to the shell. > > > > ------------------------------------------------------------------------ > > > > Please take a look or revert soon. > > Hmm, why this isn't caught by any bots? > > It seems this is windows only. I don't have a recent Windows check out to try > this. Can anyone with a Windows checkout help on this? Otherwise I'll revert my > CL. Windows apptests are FYI-only for now; they flake too often for main/cq. I have a windows box... slowly building now... sigh...
Message was sent while issue was closed.
On 2016/02/23 22:40:57, msw wrote: > On 2016/02/23 22:36:58, xhwang wrote: > > On 2016/02/23 22:32:37, msw wrote: > > > This causes numerous mojo:mus_apptests to fail on Windows: > > > > > > https://build.chromium.org/p/chromium.mojo/builders/Chromium%20Mojo%20Windows... > > > > > > [ FAILED ] Command (in 103 ms): > > > E:\b\build\slave\chromium_mojo\build\src\out\Release\mojo_runner.exe > > > --use-temporary-user-data-dir --use-x11-test-config > > > --override-use-gl-with-osmesa-for-tests > > > --gtest_filter=WindowServerTest.RootWindow mojo:mus_apptests > > > ------------------------------------------------------------------------ > > > Note: Google Test filter = WindowServerTest.RootWindow > > > [==========] Running 1 test from 1 test case. > > > [----------] Global test environment set-up. > > > [----------] 1 test from WindowServerTest > > > [ RUN ] WindowServerTest.RootWindow > > > [ERROR:child_process_base.cc(215)] Connection error to the shell. > > > [ERROR:child_process_base.cc(215)] Connection error to the shell. > > > > > > ------------------------------------------------------------------------ > > > > > > Please take a look or revert soon. > > > > Hmm, why this isn't caught by any bots? > > > > It seems this is windows only. I don't have a recent Windows check out to try > > this. Can anyone with a Windows checkout help on this? Otherwise I'll revert > my > > CL. > > Windows apptests are FYI-only for now; they flake too often for main/cq. Got it. Thanks! > I have a windows box... slowly building now... sigh... I am also syncing my windows checkout now (it was on May 2015)....
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:60001) has been created in https://chromiumcodereview.appspot.com/1728063002/ by xhwang@chromium.org. The reason for reverting is: Revert this for now to investigate crash on Windows.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
