Skip to content

Commit

Permalink
GTTF: No more FLAKY_ .
Browse files Browse the repository at this point in the history
Please note that FLAKY_ tests have been ignored anyway. When tests started
crashing, people just flipped that to DISABLED_ . Why not go straight to
DISABLED_ then, so that we avoid wasting time on stupid test prefix games?

With DISABLED_ it is clear to everyone that there is no coverage from given
test. FLAKY_ creates an illusion of coverage, while in fact the test is still
ignored.

If a FLAKY_ test fails and nobody notices, does it still make a sound? ;-)

Finally, note that gtest has a --gtest_also_run_disabled_tests if you need
to run tests manually.

TBR=jam

BUG=none

Review URL: https://codereview.chromium.org/11664013

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@174472 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
phajdan.jr@chromium.org committed Dec 21, 2012
1 parent 688c779 commit 1e09ec8
Show file tree
Hide file tree
Showing 33 changed files with 70 additions and 140 deletions.
47 changes: 0 additions & 47 deletions base/test/test_suite.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,6 @@ class TestClientInitializer : public testing::EmptyTestEventListener {

} // namespace

const char TestSuite::kStrictFailureHandling[] = "strict_failure_handling";

TestSuite::TestSuite(int argc, char** argv) : initialized_command_line_(false) {
PreInitialize(argc, argv, true);
}
Expand Down Expand Up @@ -123,45 +121,11 @@ void TestSuite::PreInitialize(int argc, char** argv,
}


// static
bool TestSuite::IsMarkedFlaky(const testing::TestInfo& test) {
return strncmp(test.name(), "FLAKY_", 6) == 0;
}

// static
bool TestSuite::IsMarkedMaybe(const testing::TestInfo& test) {
return strncmp(test.name(), "MAYBE_", 6) == 0;
}

// static
bool TestSuite::ShouldIgnoreFailure(const testing::TestInfo& test) {
if (CommandLine::ForCurrentProcess()->HasSwitch(kStrictFailureHandling))
return false;
return IsMarkedFlaky(test);
}

// static
bool TestSuite::NonIgnoredFailures(const testing::TestInfo& test) {
return test.should_run() && test.result()->Failed() &&
!ShouldIgnoreFailure(test);
}

int TestSuite::GetTestCount(TestMatch test_match) {
testing::UnitTest* instance = testing::UnitTest::GetInstance();
int count = 0;

for (int i = 0; i < instance->total_test_case_count(); ++i) {
const testing::TestCase& test_case = *instance->GetTestCase(i);
for (int j = 0; j < test_case.total_test_count(); ++j) {
if (test_match(*test_case.GetTestInfo(j))) {
count++;
}
}
}

return count;
}

void TestSuite::CatchMaybeTests() {
testing::TestEventListeners& listeners =
testing::UnitTest::GetInstance()->listeners();
Expand Down Expand Up @@ -194,17 +158,6 @@ int TestSuite::Run() {
#endif
int result = RUN_ALL_TESTS();

// If there are failed tests, see if we should ignore the failures.
if (result != 0 && GetTestCount(&TestSuite::NonIgnoredFailures) == 0)
result = 0;

// Display the number of flaky tests.
int flaky_count = GetTestCount(&TestSuite::IsMarkedFlaky);
if (flaky_count) {
printf(" YOU HAVE %d FLAKY %s\n\n", flaky_count,
flaky_count == 1 ? "TEST" : "TESTS");
}

#if defined(OS_MACOSX)
// This MUST happen before Shutdown() since Shutdown() tears down
// objects (such as NotificationService::current()) that Cocoa
Expand Down
19 changes: 0 additions & 19 deletions base/test/test_suite.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,36 +28,17 @@ class TestSuite {
TestSuite(int argc, char** argv);
virtual ~TestSuite();

// Returns true if the test is marked as flaky.
static bool IsMarkedFlaky(const testing::TestInfo& test);

// Returns true if the test is marked as failing.
static bool IsMarkedFailing(const testing::TestInfo& test);

// Returns true if the test is marked as "MAYBE_".
// When using different prefixes depending on platform, we use MAYBE_ and
// preprocessor directives to replace MAYBE_ with the target prefix.
static bool IsMarkedMaybe(const testing::TestInfo& test);

// Returns true if the test failure should be ignored.
static bool ShouldIgnoreFailure(const testing::TestInfo& test);

// Returns true if the test failed and the failure shouldn't be ignored.
static bool NonIgnoredFailures(const testing::TestInfo& test);

// Returns the number of tests where the match function returns true.
int GetTestCount(TestMatch test_match);

void CatchMaybeTests();

void ResetCommandLine();

int Run();

// A command-line flag that makes a test failure always result in a non-zero
// process exit code.
static const char kStrictFailureHandling[];

protected:
// This constructor is only accessible to specialized test suite
// implementations which need to control the creation of an AtExitManager
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/browser_keyevents_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,7 @@ IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, EditorKeyBindings) {
#endif

// See http://crbug.com/147579
IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, FLAKY_PageUpDownKeys) {
IN_PROC_BROWSER_TEST_F(BrowserKeyEventsTest, DISABLED_PageUpDownKeys) {
static const KeyEventTestData kTestPageUp = {
ui::VKEY_PRIOR, false, false, false, false,
false, false, false, false, 2,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, InstallThenCancel) {

#if defined(OS_WIN)
// http://crbug.com/141913
#define MAYBE_InstallRequiresConfirm FLAKY_InstallRequiresConfirm
#define MAYBE_InstallRequiresConfirm DISABLED_InstallRequiresConfirm
#else
#define MAYBE_InstallRequiresConfirm InstallRequiresConfirm
#endif
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/api/record/record_api_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ class RecordApiTest : public InProcessBrowserTest {
};


IN_PROC_BROWSER_TEST_F(RecordApiTest, FLAKY_CheckCapture) {
IN_PROC_BROWSER_TEST_F(RecordApiTest, DISABLED_CheckCapture) {
base::ScopedTempDir user_data_dir;
scoped_ptr<base::ListValue> result;

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/crx_installer_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ IN_PROC_BROWSER_TEST_F(
// Tests that scopes are only granted if |record_oauth2_grant_| on the prompt is
// true.
#if defined(OS_WIN)
#define MAYBE_GrantScopes FLAKY_GrantScopes
#define MAYBE_GrantScopes DISABLED_GrantScopes
#else
#define MAYBE_GrantScopes GrantScopes
#endif
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionResourceRequestPolicyTest, Iframe) {
}

#if defined(OS_MACOSX)
#define MAYBE_ExtensionAccessibleResources FLAKY_ExtensionAccessibleResources
#define MAYBE_ExtensionAccessibleResources DISABLED_ExtensionAccessibleResources
#else
#define MAYBE_ExtensionAccessibleResources ExtensionAccessibleResources
#endif
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/extensions/extension_tabs_apitest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, Tabs2) {
}

// crbug.com/149924
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, FLAKY_TabDuplicate) {
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_TabDuplicate) {
ASSERT_TRUE(RunExtensionSubtest("tabs/basics", "duplicate.html")) << message_;
}

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/history/history_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ TEST_F(HistoryTest, SetTitle) {
}

// crbug.com/159387: This test fails when daylight savings time ends.
TEST_F(HistoryTest, FLAKY_Segments) {
TEST_F(HistoryTest, DISABLED_Segments) {
ASSERT_TRUE(history_service_.get());

static const void* scope = static_cast<void*>(this);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/logging_chrome_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class RendererCrashTest : public InProcessBrowserTest,
};

// Flaky, http://crbug.com/107226 .
IN_PROC_BROWSER_TEST_F(RendererCrashTest, FLAKY_Crash) {
IN_PROC_BROWSER_TEST_F(RendererCrashTest, DISABLED_Crash) {
registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
content::NotificationService::AllSources());
ui_test_utils::NavigateToURLWithDisposition(
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/net/load_timing_observer_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class LoadTimingObserverTest : public InProcessBrowserTest {
};

// http://crbug.com/102030
IN_PROC_BROWSER_TEST_F(LoadTimingObserverTest, FLAKY_CacheHitAfterRedirect) {
IN_PROC_BROWSER_TEST_F(LoadTimingObserverTest, DISABLED_CacheHitAfterRedirect) {
ASSERT_TRUE(test_server()->Start());
GURL cached_page = test_server()->GetURL("cachetime");
std::string redirect = "server-redirect?" + cached_page.spec();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -552,7 +552,8 @@ IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, NewVersionEvent) {
}

// crbug.com/160502
IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest, FLAKY_GatherStatistics) {
IN_PROC_BROWSER_TEST_F(PerformanceMonitorBrowserTest,
DISABLED_GatherStatistics) {
GatherStatistics();

// No stats should be recorded for this CPUUsage because this was the first
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/prerender/prerender_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1350,7 +1350,7 @@ IN_PROC_BROWSER_TEST_F(PrerenderBrowserTest, PrerenderAlertAfterOnload) {
#define MAYBE_PrerenderDelayLoadPlugin DISABLED_PrerenderDelayLoadPlugin
#elif defined(OS_MACOSX)
// http://crbug.com/100514
#define MAYBE_PrerenderDelayLoadPlugin FLAKY_PrerenderDelayLoadPlugin
#define MAYBE_PrerenderDelayLoadPlugin DISABLED_PrerenderDelayLoadPlugin
#else
#define MAYBE_PrerenderDelayLoadPlugin PrerenderDelayLoadPlugin
#endif
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,7 @@ net::URLRequestJob* PrintDialogCloudTest::Factory(
}

#if defined(OS_WIN)
#define MAYBE_HandlersRegistered FLAKY_HandlersRegistered
#define MAYBE_HandlersRegistered DISABLED_HandlersRegistered
#else
#define MAYBE_HandlersRegistered HandlersRegistered
#endif
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/sync/profile_sync_service_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ TEST_F(ProfileSyncServiceTest, FailToDownloadControlTypes) {
// reenable sync. The handler should get notified of the state
// changes.
// Flaky on all platforms. http://crbug.com/154491
TEST_F(ProfileSyncServiceTest, FLAKY_DisableInvalidationsOnStop) {
TEST_F(ProfileSyncServiceTest, DISABLED_DisableInvalidationsOnStop) {
harness_.StartSyncServiceAndSetInitialSyncEnded(
true, true, true, true, syncer::STORAGE_IN_MEMORY);

Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/sync/test/integration/sync_errors_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ IN_PROC_BROWSER_TEST_F(SyncErrorTest, AuthErrorTest) {
// other auth error.
// This has been flaking a lot recently on Mac. http://crbug.com/165328
#if defined(OS_MACOSX)
#define MAYBE_XmppAuthErrorTest FLAKY_XmppAuthErrorTest
#define MAYBE_XmppAuthErrorTest DISABLED_XmppAuthErrorTest
#else
#define MAYBE_XmppAuthErrorTest XmppAuthErrorTest
#endif
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/tab_restore_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, RestoreToDifferentWindow) {
// Close a tab, open a new window, close the first window, then restore the
// tab. It should be in a new window.
// If this becomes flaky, use http://crbug.com/14774
IN_PROC_BROWSER_TEST_F(TabRestoreTest, FLAKY_BasicRestoreFromClosedWindow) {
IN_PROC_BROWSER_TEST_F(TabRestoreTest, DISABLED_BasicRestoreFromClosedWindow) {
// Navigate to url1 then url2.
ui_test_utils::NavigateToURL(browser(), url1_);
ui_test_utils::NavigateToURL(browser(), url2_);
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/panels/panel_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ class WaitForAutoResizeNarrower : public TestPanelNotificationObserver {
};

// crbug.com/160504
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, FLAKY_AutoResize) {
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, DISABLED_AutoResize) {
PanelManager* panel_manager = PanelManager::GetInstance();
panel_manager->enable_auto_sizing(true);
// Bigger space is needed by this test.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class GuestModeOptionsBrowserTest : public options::OptionsBrowserTest {
}
};

IN_PROC_BROWSER_TEST_F(GuestModeOptionsBrowserTest, FLAKY_LoadOptionsByURL) {
IN_PROC_BROWSER_TEST_F(GuestModeOptionsBrowserTest, DISABLED_LoadOptionsByURL) {
NavigateToSettings();
VerifyTitle();
VerifyNavbar();
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ui/webui/options/options_ui_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ void OptionsBrowserTest::VerifyTitle() {
}

// Flaky, see http://crbug.com/119671.
IN_PROC_BROWSER_TEST_F(OptionsBrowserTest, FLAKY_LoadOptionsByURL) {
IN_PROC_BROWSER_TEST_F(OptionsBrowserTest, DISABLED_LoadOptionsByURL) {
NavigateToSettings();
VerifyTitle();
VerifyNavbar();
Expand Down
2 changes: 1 addition & 1 deletion chrome/common/time_format_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ TEST(TimeFormat, FormatTime) {
}

// crbug.com/159388: This test fails when daylight savings time ends.
TEST(TimeFormat, FLAKY_RelativeDate) {
TEST(TimeFormat, RelativeDate) {
base::Time now = base::Time::Now();
string16 today_str = TimeFormat::RelativeDate(now, NULL);
EXPECT_EQ(ASCIIToUTF16("Today"), today_str);
Expand Down
2 changes: 1 addition & 1 deletion chrome/test/data/webui/ntp4.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ TEST_F('NTP4WebUITest', 'TestBrowsePages', function() {
});

// http://crbug.com/118944
TEST_F('NTP4WebUITest', 'FLAKY_NTPHasThumbnails', function() {
TEST_F('NTP4WebUITest', 'DISABLED_NTPHasThumbnails', function() {
var mostVisited = document.querySelectorAll('.most-visited');
assertEquals(8, mostVisited.length, 'There should be 8 most visited tiles.');

Expand Down
10 changes: 4 additions & 6 deletions chrome/test/data/webui/sandboxstatus_browsertest.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@ SandboxStatusUITest.prototype = {
// the proper way to address such failures is to install the SUID
// sandbox. See:
// http://code.google.com/p/chromium/wiki/LinuxSUIDSandboxDevelopment
// - PLEASE DO NOT DISABLE THIS TEST. If you can't figure out how to
// get the SUID sandbox installed, please mark the test FLAKY_ so we
// can get clear information on where the sandbox is and isn't installed.
// - PLEASE DO NOT GLOBALLY DISABLE THIS TEST.
// SUID sandbox is currently incompatible with AddressSanitizer,
// see http://crbug.com/137653.
GEN('#if defined(OS_LINUX) && !defined(ADDRESS_SANITIZER)');
Expand All @@ -40,7 +38,7 @@ GEN('#endif');
/**
* Test if the SUID sandbox is enabled.
*
* TODO(bradchen): Remove FLAKY_ and flip polarity of this test once
* TODO(bradchen): Remove DISABLED_ and flip polarity of this test once
* the SUID sandbox is enabled on the Chromium bots. In the mean time
* this test will make it clear that the enabling steps are effective.
*/
Expand All @@ -58,7 +56,7 @@ TEST_F('SandboxStatusUITest', 'MAYBE_testSUIDSandboxEnabled', function() {
// The seccomp-bpf sandbox is also not compatible with ASAN.
GEN('#if defined(OS_LINUX) && !defined(ADDRESS_SANITIZER)');
GEN('# define MAYBE_testBPFSandboxEnabled \\');
GEN(' FLAKY_testBPFSandboxEnabled');
GEN(' DISABLED_testBPFSandboxEnabled');
GEN('#else');
GEN('# define MAYBE_testBPFSandboxEnabled \\');
GEN(' DISABLED_testBPFSandboxEnabled');
Expand All @@ -67,7 +65,7 @@ GEN('#endif');
/**
* Test if the seccomp-bpf sandbox is enabled.
* We know that some machines lack kernel support for this. So we mark
* it as FLAKY_.
* it as DISABLED_.
* It's very convenient to quickly be able to check whether tests ran with
* or without the Seccomp BPF sandbox through this mechanism.
*/
Expand Down
2 changes: 1 addition & 1 deletion chrome/test/ppapi/ppapi_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(URLRequest_CreateAndIsURLRequestInfo)
// Timing out on Windows. http://crbug.com/129571
#if defined(OS_WIN)
#define MAYBE_URLRequest_CreateAndIsURLRequestInfo \
FLAKY_URLRequest_CreateAndIsURLRequestInfo
DISABLED_URLRequest_CreateAndIsURLRequestInfo
#else
#define MAYBE_URLRequest_CreateAndIsURLRequestInfo \
URLRequest_CreateAndIsURLRequestInfo
Expand Down
2 changes: 1 addition & 1 deletion chrome_frame/test/navigation_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ class NavigationTest : public MockIEEventSinkTest, public testing::Test {

// Test navigation to a disallowed gcf: url with file scheme.
// Times out sporadically; http://crbug.com/119718.
TEST_F(NavigationTest, FLAKY_GcfProtocol1) {
TEST_F(NavigationTest, DISABLED_GcfProtocol1) {
// Make sure that we are not accidently enabling gcf protocol.
SetConfigBool(kAllowUnsafeURLs, false);
TestDisAllowedUrl(L"gcf:file:///C:/");
Expand Down
Loading

0 comments on commit 1e09ec8

Please sign in to comment.