Skip to content

GitLab

T
tangled
Commit fe992bf0 authored by scottbyer@chromium.org's avatar scottbyer@chromium.org
Browse files
Options

Revert 144488 - For unit tests, track additions to AtExitManager and warn. 

While trying to bullet proof a unit test, I had trouble getting very far when
running all tests in shuffle mode. Tracked that back to a few other tests doing
stuff that accessed Singleton()s outside of a test-scoped
ShadowingAtExitManager. Seemed to me that should be an invariant around any
unit test, so created this towards that end, hopefully helping stabilize out
unit_tests a bit more.

BUG=133403


Review URL: https://chromiumcodereview.appspot.com/10582012

TBR=scottbyer@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10834010

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@148405 0039d316-1c4b-4281-b951-d872f2087c98
parent c9236ae7
Hide whitespace changes
Inline Side-by-side
Showing with 17 additions and 102 deletions
base/at_exit.cc
View file @ fe992bf0
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
......@@ -79,13 +79,4 @@ AtExitManager::AtExitManager(bool shadow) : next_manager_(g_top_manager) {
g_top_manager = this;
}
// static
AtExitManager* AtExitManager::current() {
return g_top_manager;
}
size_t AtExitManager::CallbackStackSize() const {
return stack_.size();
}
} // namespace base
base/at_exit.h
View file @ fe992bf0
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
......@@ -14,8 +14,6 @@
namespace base {
class TestWatchAtExitManager;
// This class provides a facility similar to the CRT atexit(), except that
// we control when the callbacks are executed. Under Windows for a DLL they
// happen at a really bad time and under the loader lock. This facility is
......@@ -59,11 +57,6 @@ class BASE_EXPORT AtExitManager {
explicit AtExitManager(bool shadow);
private:
friend class TestWatchAtExitManager;
static AtExitManager* current();
size_t CallbackStackSize() const;
base::Lock lock_;
std::stack<base::Closure> stack_;
AtExitManager* next_manager_; // Stack of managers to allow shadowing.
......
base/test/test_suite.cc
View file @ fe992bf0
......@@ -17,7 +17,6 @@
#include "base/path_service.h"
#include "base/process_util.h"
#include "base/test/multiprocess_test.h"
#include "base/test/test_switches.h"
#include "base/test/test_timeouts.h"
#include "base/time.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -78,46 +77,6 @@ class TestClientInitializer : public testing::EmptyTestEventListener {
} // namespace
namespace base {
class TestWatchAtExitManager : public testing::EmptyTestEventListener {
public:
TestWatchAtExitManager() { }
~TestWatchAtExitManager() { }
virtual void OnTestStart(const testing::TestInfo& test_info) OVERRIDE {
initial_top_manager_ = AtExitManager::current();
at_exit_stack_size_ = initial_top_manager_->CallbackStackSize();
}
virtual void OnTestEnd(const testing::TestInfo& test_info) OVERRIDE {
AtExitManager* new_top_manager = AtExitManager::current();
size_t new_stack_size = new_top_manager->CallbackStackSize();
if (initial_top_manager_ != new_top_manager) {
ADD_FAILURE() << "The current AtExitManager has changed across the "
"test " << test_info.test_case_name() << "." << test_info.name() <<
" most likely because one was created without being destroyed.";
} else if (new_stack_size != at_exit_stack_size_) {
// TODO(scottbyer): clean up all the errors that result from this and
// turn this into a test failure with
// ADD_FAILURE(). http://crbug.com/133403
LOG(WARNING) <<
"AtExitManager: items were added to the callback list by " <<
test_info.test_case_name() << "." << test_info.name() <<
". Global state should be cleaned up before a test exits.";
}
}
private:
AtExitManager* initial_top_manager_;
size_t at_exit_stack_size_;
DISALLOW_COPY_AND_ASSIGN(TestWatchAtExitManager);
};
} // namespace base
const char TestSuite::kStrictFailureHandling[] = "strict_failure_handling";
TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false) {
......@@ -219,12 +178,6 @@ void TestSuite::ResetCommandLine() {
listeners.Append(new TestClientInitializer);
}
void TestSuite::WatchAtExitManager() {
testing::TestEventListeners& listeners =
testing::UnitTest::GetInstance()->listeners();
listeners.Append(new TestWatchAtExitManager);
}
// Don't add additional code to this method. Instead add it to
// Initialize(). See bug 6436.
int TestSuite::Run() {
......@@ -349,15 +302,6 @@ void TestSuite::Initialize() {
CatchMaybeTests();
ResetCommandLine();
// Don't watch for AtExit items being added if we're running as a child
// process (e.g., browser_tests or interactive_ui_tests).
if (!CommandLine::ForCurrentProcess()->HasSwitch(
switches::kSingleProcessTestsFlag) &&
!CommandLine::ForCurrentProcess()->HasSwitch(
switches::kSingleProcessChromeFlag)) {
WatchAtExitManager();
}
TestTimeouts::Initialize();
}
......
base/test/test_suite.h
View file @ fe992bf0
......@@ -52,8 +52,6 @@ class TestSuite {
void ResetCommandLine();
void WatchAtExitManager();
int Run();
// A command-line flag that makes a test failure always result in a non-zero
......
base/test/test_switches.cc
View file @ fe992bf0
......@@ -4,20 +4,9 @@
#include "base/test/test_switches.h"
namespace switches {
// Run Chromium in single process mode.
const char kSingleProcessChromeFlag[] = "single-process";
// Run the tests and the launcher in the same process. Useful for debugging a
// specific test in a debugger.
const char kSingleProcessTestsFlag[] = "single_process";
// Time (in milliseconds) that the tests should wait before timing out.
// TODO(phajdan.jr): Clean up the switch names.
const char kTestLargeTimeout[] = "test-large-timeout";
const char kTestTinyTimeout[] = "test-tiny-timeout";
const char kUiTestActionTimeout[] = "ui-test-action-timeout";
const char kUiTestActionMaxTimeout[] = "ui-test-action-max-timeout";
} // namespace switches
const char switches::kTestLargeTimeout[] = "test-large-timeout";
const char switches::kTestTinyTimeout[] = "test-tiny-timeout";
const char switches::kUiTestActionTimeout[] = "ui-test-action-timeout";
const char switches::kUiTestActionMaxTimeout[] = "ui-test-action-max-timeout";
base/test/test_switches.h
View file @ fe992bf0
......@@ -9,8 +9,6 @@ namespace switches {
// All switches in alphabetical order. The switches should be documented
// alongside the definition of their values in the .cc file.
extern const char kSingleProcessChromeFlag[];
extern const char kSingleProcessTestsFlag[];
extern const char kTestLargeTimeout[];
extern const char kTestTinyTimeout[];
extern const char kUiTestActionTimeout[];
......
chrome/test/base/in_process_browser_test.cc
View file @ fe992bf0
......@@ -13,7 +13,6 @@
#include "base/path_service.h"
#include "base/string_number_conversions.h"
#include "base/test/test_file_util.h"
#include "base/test/test_switches.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/io_thread.h"
#include "chrome/browser/lifetime/application_lifetime.h"
......@@ -284,8 +283,8 @@ CommandLine InProcessBrowserTest::GetCommandLineForRelaunch() {
CommandLine::SwitchMap switches =
CommandLine::ForCurrentProcess()->GetSwitches();
switches.erase(switches::kUserDataDir);
switches.erase(switches::kSingleProcessTestsFlag);
switches.erase(switches::kSingleProcessChromeFlag);
switches.erase(test_launcher::kSingleProcessTestsFlag);
switches.erase(test_launcher::kSingleProcessTestsAndChromeFlag);
new_command_line.AppendSwitch(ChromeTestSuite::kLaunchAsBrowser);
#if defined(USE_AURA)
......
content/public/test/test_launcher.h
View file @ fe992bf0
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
......@@ -25,6 +25,8 @@ extern const char kGTestListTestsFlag[];
extern const char kGTestRepeatFlag[];
extern const char kGTestRunDisabledTestsFlag[];
extern const char kGTestOutputFlag[];
extern const char kSingleProcessTestsFlag[];
extern const char kSingleProcessTestsAndChromeFlag[];
extern const char kHelpFlag[];
// Flag that causes only the kEmptyTestName test to be run.
......
content/test/test_launcher.cc
View file @ fe992bf0
......@@ -19,7 +19,6 @@
#include "base/string_number_conversions.h"
#include "base/string_util.h"
#include "base/test/test_suite.h"
#include "base/test/test_switches.h"
#include "base/test/test_timeouts.h"
#include "base/time.h"
#include "base/utf_string_conversions.h"
......@@ -338,7 +337,7 @@ int RunTest(TestLauncherDelegate* launcher_delegate,
// tests unless this flag was specified to the browser test executable.
new_cmd_line.AppendSwitch("gtest_also_run_disabled_tests");
new_cmd_line.AppendSwitchASCII("gtest_filter", test_name);
new_cmd_line.AppendSwitch(switches::kSingleProcessTestsFlag);
new_cmd_line.AppendSwitch(kSingleProcessTestsFlag);
// Do not let the child ignore failures. We need to propagate the
// failure status back to the parent.
......@@ -561,6 +560,8 @@ const char kGTestRepeatFlag[] = "gtest_repeat";
const char kGTestRunDisabledTestsFlag[] = "gtest_also_run_disabled_tests";
const char kGTestOutputFlag[] = "gtest_output";
const char kSingleProcessTestsFlag[] = "single_process";
const char kSingleProcessTestsAndChromeFlag[] = "single-process";
// The following is kept for historical reasons (so people that are used to
// using it don't get surprised).
const char kChildProcessFlag[] = "child";
......@@ -590,12 +591,12 @@ int LaunchTests(TestLauncherDelegate* launcher_delegate,
// terrible UI. Instead, there should be some sort of signal flag on the
// command line, with all subsequent arguments passed through to the
// underlying browser.
if (command_line->HasSwitch(switches::kSingleProcessTestsFlag) ||
command_line->HasSwitch(switches::kSingleProcessChromeFlag) ||
if (command_line->HasSwitch(kSingleProcessTestsFlag) ||
command_line->HasSwitch(kSingleProcessTestsAndChromeFlag) ||
command_line->HasSwitch(kGTestListTestsFlag) ||
command_line->HasSwitch(kGTestHelpFlag)) {
#if defined(OS_WIN)
if (command_line->HasSwitch(switches::kSingleProcessTestsFlag)) {
if (command_line->HasSwitch(kSingleProcessTestsFlag)) {
sandbox::SandboxInterfaceInfo sandbox_info;
content::InitializeSandboxInfo(&sandbox_info);
content::InitializeSandbox(&sandbox_info);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment