Provide a way for tests to guard against RunLoop::Run() timing-out, and set a default in ScopedTaskEnvironment. |
||||
Issue descriptionTests which use RunLoop::Run() to execute until a particular condition occurs are prone to hanging, if the relevant condition never occurs, in which case we end up relying on the TestLauncher spotting that the batch sub-process has "hung", and terminating it. This makes debugging hanging tests, especially flakily hanging ones, difficult, since we don't necessarily know where they hung. We propose to add support for a default timeout to be specified per-thread, to apply to all subsequent Run() calls. Tests (or more usually test suites or base classes) will use this to timeout rather than hang, if the expected condition does not occur. We also propose to set a default timeout when a ScopedTaskEnvironment is in-use.
,
Jan 3
Issues with Run() timeouts and MOCK_TIME interactions: 1. Some ScopedTaskEnvironment tests expect Run() to fast-forward by more than the timeout, to execute a task (e.g. see ScopedTaskEnvironmentMockedTime.RunLoopDriveable). These need real-time Run() timeouts, or for the timeout to be disabled during the test. 2. MainThreadHasPendingTasks() returns true for MOCK_TIME main thread if there is a Run() timeout delayed task pending, even if it was cancelled, because of the way we're currently implementing it.
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/218f0e426a83542ad428c7cceebaf4c76066a5bc commit 218f0e426a83542ad428c7cceebaf4c76066a5bc Author: Wez <wez@chromium.org> Date: Fri Jan 04 01:52:09 2019 Provide webkit_unit_tests environment via a TestSuite specialization. Update the blink/renderer/controller/tests/run_all_tests.cc e.g. used by webkit_unit_tests, to provide the expected test environment by deriving from base::TestSuite and overriding Initialize() and Shutdown(). Bug: 918724 Change-Id: I711e29f70517ec793b08441b05fa82e33e461613 Reviewed-on: https://chromium-review.googlesource.com/c/1395194 Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#619857} [modify] https://crrev.com/218f0e426a83542ad428c7cceebaf4c76066a5bc/third_party/blink/renderer/controller/tests/run_all_tests.cc
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7e1eb20fc11580f69dc22f67600333ae5425901c commit 7e1eb20fc11580f69dc22f67600333ae5425901c Author: Wez <wez@chromium.org> Date: Tue Jan 08 01:48:45 2019 Provide media_blink_unittests environment via TestSuite specialization. Provide the expected test environment by deriving from base::TestSuite and overriding Initialize() and Shutdown(). Bug: 918724 Change-Id: I5dfb1a2cd5a7df3248e7b2dfbfab3236debd91f5 Reviewed-on: https://chromium-review.googlesource.com/c/1395780 Reviewed-by: John Rummell <jrummell@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#620574} [modify] https://crrev.com/7e1eb20fc11580f69dc22f67600333ae5425901c/media/blink/run_all_unittests.cc
,
Jan 8
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/83a4b801019860d7f2cd0451a1f5491a8ce5ba33 commit 83a4b801019860d7f2cd0451a1f5491a8ce5ba33 Author: Wez <wez@chromium.org> Date: Tue Jan 08 21:53:36 2019 Move video decode accelerator test environment init to Initialize(). This allows ScopedTaskEnvironment to take advantage of state initialized by base::TestSuite::Initialize, such as the TestTimeouts, as required by https://chromium-review.googlesource.com/c/chromium/src/+/1319344 Bug: 918724 Change-Id: Ic7e811f95581e7d07a26acd3e29e27442a9828b5 Reviewed-on: https://chromium-review.googlesource.com/c/1400346 Reviewed-by: David Staessens <dstaessens@chromium.org> Reviewed-by: Dan Sanders <sandersd@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#620892} [modify] https://crrev.com/83a4b801019860d7f2cd0451a1f5491a8ce5ba33/media/gpu/video_decode_accelerator_unittest.cc
,
Jan 9
,
Jan 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6 commit d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6 Author: Wez <wez@chromium.org> Date: Wed Jan 09 03:07:03 2019 Apply a default timeout to RunLoop::Run() calls on test main threads. Sheriffs: This change is intended to expose tests which Run() for an excessive amount of time. Please consider disabling tests which hit the new Run() timeouts, rather than reverting this CL. We add RunLoop::ScopedRunTimeoutForTest, which configures the calling thread with a timeout, and optional callback, to apply to all Run() calls made while it is in-scope. If a Run() is not Quit() by the caller within the specified timeout then the loop is implicitly quit, and the optional callback is run, if provided. ScopedTaskEnvironment now applies a default ScopedRunTimeoutForTest set to invoke LOG(FATAL) if any Run() on the test's main thread runs for more than TestTimeouts::action_max_timeout(). Bug: 918724 Change-Id: I080b78e193202da0c9e3deee3a201ef1619cfd37 Reviewed-on: https://chromium-review.googlesource.com/c/1319344 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Albert J. Wong <ajwong@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#621022} [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/base/run_loop.cc [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/base/run_loop.h [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/base/run_loop_unittest.cc [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/base/test/scoped_task_environment.cc [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/base/test/scoped_task_environment.h [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/base/test/scoped_task_environment_unittest.cc [modify] https://crrev.com/d9e4cb77324a3d4e0dfd6b599ce34e1224fdedb6/docs/threading_and_tasks.md
,
Jan 9
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a72257bad57d95dbc42dc00c8ebc588e2fd8586f commit a72257bad57d95dbc42dc00c8ebc588e2fd8586f Author: Wez <wez@chromium.org> Date: Wed Jan 09 18:58:29 2019 Initialize base::TestTimeouts in media_pipeline_integration_fuzzer. The PipelineIntegrationTestBase uses ScopedTaskEnvironment internally, which now requires that TestTimeouts were Initialize()d for the process. TBR=xhwang Bug: 920111 , 918724 Change-Id: Idbf0197eb160fe8af2195f50e2e3da58f25a17a5 Reviewed-on: https://chromium-review.googlesource.com/c/1403186 Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#621247} [modify] https://crrev.com/a72257bad57d95dbc42dc00c8ebc588e2fd8586f/media/test/BUILD.gn [modify] https://crrev.com/a72257bad57d95dbc42dc00c8ebc588e2fd8586f/media/test/pipeline_integration_fuzzertest.cc
,
Jan 9
,
Jan 15
ajwong, gab: Run() timeouts don't seem to be causing too much havoc, and I've verified that they're firing in some of our known-hangy tests, when we temporarily re-enabled them. We should consider enabling Run() timeouts by default for more suites, e.g. having base::TestSuite set it, and having base::ScopedTaskEnvironment disable it in the case of MOCK_TIME. It'd also be good to reduce the timeout (currently 30s in Release, 45s in Debug). However, I think the work described in this bug is done -> Fixed :)
,
Jan 15
Awesome! Thanks Wez for pushing this through, it will be *much* simpler to identify tests that timeout in a RunLoop::Run() as they'll now have a failure and stack at the location of the hung Run() rather than be killed by the test host =P! Let's open a Hotlist-GoodFirstBug to reduce the default timeout though?
,
Jan 15
Thanks Gab - filed issue 922098 for follow-up work. :) |
||||
►
Sign in to add a comment |
||||
Comment 1 by bugdroid1@chromium.org
, Jan 3