Check for global resources that are initialized by tests but not torn down (e.g. TaskScheduler) |
|||
Issue descriptionWe regularly find tests flaking because they expect some static-global to be uninitialized, but instead find that some prior test initialized it, and forgot to tear it down. e.g. we often have tests start to flake or fail because ScopedTaskEnvironment is instantiated when TaskScheduler::GetInstance() is already non-null - various classes which would initialize TaskScheduler in production code only do so if it is not already set, assuming that test code will always provide a ScopedTaskEnvironment where needed. Ideally we should be able to identify whether the code is being run in a test binary, or a production one, and adjust the initialization expectations accordingly. e.g. the content::ChildProcess could initialize the TaskScheduler only if a base::IsTestProcess() returns false, and otherwise assume that calling-code must provide one.
,
Aug 20
Re #1: Yes, I definitely don't want to encourage run-time tests for test-specific logic, so we'd need some presubmit to ensure appropriate //base OWNERS review for new uses. I considered the idea of verifying !TaskScheduler::GetInstance() at the end of each test, but that requires that we implement that for every one of our unit-test suites, of which there are several, which seems easy-to-miss as new suites are added. OTOH that check would catch this class of inter-test leaks, and is a lot better than the current situation, so SGTM.
,
Aug 20
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9e0bfb905e341606c620b4e58926feddfbe001e commit a9e0bfb905e341606c620b4e58926feddfbe001e Author: Wez <wez@chromium.org> Date: Thu Aug 23 00:07:07 2018 Add missing ScopedTaskEnvironment to stop tests leaking TaskScheduler. Bug: 875486 Change-Id: I448361ccdb91b83ab71c6eda34066e6e76c278de Reviewed-on: https://chromium-review.googlesource.com/1185490 Reviewed-by: Raymes Khoury <raymes@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#585323} [modify] https://crrev.com/a9e0bfb905e341606c620b4e58926feddfbe001e/content/renderer/pepper/pepper_broker_unittest.cc
,
Aug 23
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a2a4e75d2752687d91e1272450bdcc6e48b33739 commit a2a4e75d2752687d91e1272450bdcc6e48b33739 Author: Wez <wez@chromium.org> Date: Thu Aug 23 23:21:39 2018 Check that TaskScheduler is not leaked between tests, or test-cases. - Move some APIs off of base::TestSuite, that we unnecessarily public. - Rename the various internal TestEventListeners to express purpose. - Add CheckForLeakedGlobals check, and DisableCheckForLeakedGlobals API. The latter is called by the content::ContentTestSuiteBase, since the various browser- and ui-tests running under that suite base run more like the browser itself, and so are expected to let global singletons leak. Bug: 875486 Change-Id: If749dfe8530abc4a5c13a840bbf8258d48c377c5 Reviewed-on: https://chromium-review.googlesource.com/1182612 Reviewed-by: Scott Violet <sky@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#585652} [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/base/test/test_suite.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/base/test/test_suite.h [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/chrome/test/base/chrome_test_launcher.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/chrome/test/base/interactive_ui_tests_main.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/chromecast/app/cast_test_launcher.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/content/test/content_test_launcher.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/extensions/shell/test/shell_test_launcher_delegate.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/headless/test/headless_test_launcher.cc [modify] https://crrev.com/a2a4e75d2752687d91e1272450bdcc6e48b33739/webrunner/browser/webrunner_test_launcher.cc
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78ea4f0fa239719abf86af30f39c73ccbb15b759 commit 78ea4f0fa239719abf86af30f39c73ccbb15b759 Author: Hayato Ito <hayato@chromium.org> Date: Fri Aug 24 04:49:46 2018 Revert "Check that TaskScheduler is not leaked between tests, or test-cases." This reverts commit a2a4e75d2752687d91e1272450bdcc6e48b33739. Reason for revert: [sheriff] a culprit for cronet_tests failing on multiple builders See https://crbug.com/877355 for details. Original change's description: > Check that TaskScheduler is not leaked between tests, or test-cases. > > - Move some APIs off of base::TestSuite, that we unnecessarily public. > - Rename the various internal TestEventListeners to express purpose. > - Add CheckForLeakedGlobals check, and DisableCheckForLeakedGlobals API. > The latter is called by the content::ContentTestSuiteBase, since the > various browser- and ui-tests running under that suite base run more > like the browser itself, and so are expected to let global singletons > leak. > > Bug: 875486 > Change-Id: If749dfe8530abc4a5c13a840bbf8258d48c377c5 > Reviewed-on: https://chromium-review.googlesource.com/1182612 > Reviewed-by: Scott Violet <sky@chromium.org> > Reviewed-by: Gabriel Charette <gab@chromium.org> > Commit-Queue: Wez <wez@chromium.org> > Cr-Commit-Position: refs/heads/master@{#585652} TBR=sky@chromium.org,wez@chromium.org,gab@chromium.org Change-Id: I906458f276100d9f924a0a78c7cca3a55bcb28b6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 875486 Reviewed-on: https://chromium-review.googlesource.com/1187982 Reviewed-by: Hayato Ito <hayato@chromium.org> Commit-Queue: Hayato Ito <hayato@chromium.org> Cr-Commit-Position: refs/heads/master@{#585680} [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/base/test/test_suite.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/base/test/test_suite.h [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/chrome/test/base/chrome_test_launcher.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/chrome/test/base/interactive_ui_tests_main.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/chromecast/app/cast_test_launcher.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/content/test/content_test_launcher.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/extensions/shell/test/shell_test_launcher_delegate.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/headless/test/headless_test_launcher.cc [modify] https://crrev.com/78ea4f0fa239719abf86af30f39c73ccbb15b759/webrunner/browser/webrunner_test_launcher.cc
,
Aug 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3189010800a3d171ada31068ef82fbda172e365b commit 3189010800a3d171ada31068ef82fbda172e365b Author: Wez <wez@chromium.org> Date: Fri Aug 24 23:57:42 2018 Check that TaskScheduler is not leaked between tests, or test-cases. This is a reland of a2a4e75d2752687d91e1272450bdcc6e48b33739 with a work-around for cronet_tests' sharing libbase.so with libcronet.so in component builds. - Move some APIs off of base::TestSuite, that we unnecessarily public. - Rename the various internal TestEventListeners to express purpose. - Add CheckForLeakedGlobals check, and DisableCheckForLeakedGlobals API. The latter is called by the content::ContentTestSuiteBase, since the various browser- and ui-tests running under that suite base run more like the browser itself, and so are expected to let global singletons leak. If tests fail or flake due to this patch, then this indicates that the test is actually leaking global state that may break other tests if they happen to run in the same process, leading to hard-to-diagnose flakes. There are two main failure cases: If the test directly or indirectly uses TaskScheduler, but does not create a ScopedTaskEnvironment, then please add one to the test, or to the test base-class, as appropriate, to fix it. If the test suite cannot be fixed in this way then please add a call to DisableCheckForLeakedGlobals(), with a comment linking to a bug for the issue. TBR: gab, sky, mef Bug: 875486, 877355 , 744567 Change-Id: Iaea38d24ede271c248a3abb0b3f7ee931c2538f5 Reviewed-on: https://chromium-review.googlesource.com/1187783 Reviewed-by: Wez <wez@chromium.org> Commit-Queue: Wez <wez@chromium.org> Cr-Commit-Position: refs/heads/master@{#586065} [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/base/test/test_suite.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/base/test/test_suite.h [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/chrome/test/base/chrome_test_launcher.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/chrome/test/base/interactive_ui_tests_main.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/chromecast/app/cast_test_launcher.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/components/cronet/BUILD.gn [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/components/cronet/run_all_unittests.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/content/test/content_test_launcher.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/extensions/shell/test/shell_test_launcher_delegate.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/headless/test/headless_test_launcher.cc [modify] https://crrev.com/3189010800a3d171ada31068ef82fbda172e365b/webrunner/browser/webrunner_test_launcher.cc
,
Aug 25
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/caf38a109f73e6b74ae616a3c5df2af715856284 commit caf38a109f73e6b74ae616a3c5df2af715856284 Author: Victor Costan <pwnall@chromium.org> Date: Mon Aug 27 09:49:45 2018 cronet: Add ScopedTaskEnvironment to native tests. This is needed to fix cronet_unittests_android on the android_cronet_tester bot after https://crrev.com/c/1187783. TBR=mef Bug: 877868 , 875486 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester Change-Id: If59e993e3e6b19cb54f04c2259014c34d6ad4592 Reviewed-on: https://chromium-review.googlesource.com/1189712 Reviewed-by: Victor Costan <pwnall@chromium.org> Commit-Queue: Victor Costan <pwnall@chromium.org> Cr-Commit-Position: refs/heads/master@{#586221} [modify] https://crrev.com/caf38a109f73e6b74ae616a3c5df2af715856284/components/cronet/native/test/engine_test.cc [modify] https://crrev.com/caf38a109f73e6b74ae616a3c5df2af715856284/components/cronet/native/test/url_request_test.cc
,
Aug 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c752f9eff8445ee3743f20a812e4b0e578ffa3f commit 8c752f9eff8445ee3743f20a812e4b0e578ffa3f Author: Markus Heintz <markusheintz@chromium.org> Date: Mon Aug 27 15:13:01 2018 Revert "cronet: Add ScopedTaskEnvironment to native tests." This reverts commit caf38a109f73e6b74ae616a3c5df2af715856284. Reason for revert: Finit identified this CL as reason for breaking cronet_tests: - cronet_tests failing on chromium.mac/Mac10.13 Tests (dbg) - cronet_tests failing on chromium.linux/Linux Tests (dbg)(1) - cronet_tests failing on chromium.linux/Linux Tests (dbg)(1)(32) e.g. https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29/74010 Original change's description: > cronet: Add ScopedTaskEnvironment to native tests. > > This is needed to fix cronet_unittests_android on the > android_cronet_tester bot after https://crrev.com/c/1187783. > > TBR=mef > > Bug: 877868 , 875486 > Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester > Change-Id: If59e993e3e6b19cb54f04c2259014c34d6ad4592 > Reviewed-on: https://chromium-review.googlesource.com/1189712 > Reviewed-by: Victor Costan <pwnall@chromium.org> > Commit-Queue: Victor Costan <pwnall@chromium.org> > Cr-Commit-Position: refs/heads/master@{#586221} TBR=wez@chromium.org,mef@chromium.org,pwnall@chromium.org Change-Id: Id1975c8bec0d0c863bbc36c23516e0c1cdc47ace No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 877868 , 875486 Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;master.tryserver.chromium.android:android_cronet_tester Reviewed-on: https://chromium-review.googlesource.com/1190622 Reviewed-by: Markus Heintz <markusheintz@chromium.org> Commit-Queue: Markus Heintz <markusheintz@chromium.org> Cr-Commit-Position: refs/heads/master@{#586265} [modify] https://crrev.com/8c752f9eff8445ee3743f20a812e4b0e578ffa3f/components/cronet/native/test/engine_test.cc [modify] https://crrev.com/8c752f9eff8445ee3743f20a812e4b0e578ffa3f/components/cronet/native/test/url_request_test.cc
,
Aug 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/eb4e995a94dd76f8cb86d00a95ba7535cbfae73f commit eb4e995a94dd76f8cb86d00a95ba7535cbfae73f Author: Wez <wez@chromium.org> Date: Tue Aug 28 22:15:47 2018 Explain !GetInstance() check in ScopedTaskEnvironment constructor. Add explanatory text to clarify why this fails, that it may be caused by an earlier test, and that that can only be the case if the check in base::TestSuite was explicitly disabled. Bug: 875486 Change-Id: I01ba5c496d6bff9b6d664fa0a79627ba4e5af7da Reviewed-on: https://chromium-review.googlesource.com/1192323 Commit-Queue: Wez <wez@chromium.org> Reviewed-by: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#586886} [modify] https://crrev.com/eb4e995a94dd76f8cb86d00a95ba7535cbfae73f/base/test/scoped_task_environment.cc |
|||
►
Sign in to add a comment |
|||
Comment 1 by gab@chromium.org
, Aug 20