base/task_scheduler/test_utils.h - Issue 1705943002: TaskScheduler [5/9] Task Tracker

Side by Side Diff: base/task_scheduler/test_utils.h

Issue 1705943002: TaskScheduler [5/9] Task Tracker (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@s_3_pq

Patch Set: self review Created 4 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View unified diff | Download patch

« base/task_scheduler/task_tracker.cc ('K') | « base/task_scheduler/task_traits.cc ('k') | tools/metrics/histograms/histograms.xml » ('j') | tools/metrics/histograms/histograms.xml » ('J')

OLD	NEW
1 // Copyright 2016 The Chromium Authors. All rights reserved.	1 // Copyright 2016 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #ifndef BASE_TASK_SCHEDULER_TEST_UTILS_H_	5 #ifndef BASE_TASK_SCHEDULER_TEST_UTILS_H_

6 #define BASE_TASK_SCHEDULER_TEST_UTILS_H_	6 #define BASE_TASK_SCHEDULER_TEST_UTILS_H_

7	7

8 #include "base/logging.h"	8 #include "base/logging.h"

9 #include "build/build_config.h"	9 #include "build/build_config.h"

10 #include "testing/gtest/include/gtest/gtest.h"	10 #include "testing/gtest/include/gtest/gtest.h"

11	11

12 // Death tests misbehave on Android.	12 // Death tests misbehave on Android.

13 #if DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)	13 #if DCHECK_IS_ON() && defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)

14 #define EXPECT_DCHECK_DEATH(statement, regex) EXPECT_DEATH(statement, regex)	14 #define EXPECT_DCHECK_DEATH(statement, regex) EXPECT_DEATH(statement, regex)

15 #else	15 #else

16 #define EXPECT_DCHECK_DEATH(statement, regex)	16 #define EXPECT_DCHECK_DEATH(statement, regex) statement
	gab 2016/03/21 17:42:30 Thought you'd convinced me not to do this? What ch Thought you'd convinced me not to do this? What changed? fdoray 2016/03/21 19:08:07 I explained why we sometimes put multiple statemen Show quoted text On 2016/03/21 17:42:30, gab wrote: > Thought you'd convinced me not to do this? What changed? I explained why we sometimes put multiple statements inside the EXPECT_DCHECK_DEATH macro [1], but I never said that we shouldn't execute \|statement\| when !DCHECK_IS_ON(). Pros/cons: We should always run \|statement\|: - To be consistent with EXPECT_DEBUG_DEATH() [2]. - To be able to verify the state after a DCHECK failure. For example, I would like to verify that BLOCK_SHUTDOWN tasks posted after shutdown don't run in a !DCHECK_IS_ON() build. We should only run \|statement\| when DCHECK_IS_ON(): - There is no need to verify what happens after a DCHECK because the style guide says that a DCHECK verifies a condition that "must always be true". The state after a DCHECK might be invalid. Note that existing SchedulerLock tests pass regardless of whether \|statement\| is executed. [1] https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... [2] https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... gab 2016/03/21 19:53:25 Hmm, this is surprising, that probably at the very Show quoted text On 2016/03/21 19:08:07, fdoray wrote: > On 2016/03/21 17:42:30, gab wrote: > > Thought you'd convinced me not to do this? What changed? > > I explained why we sometimes put multiple statements inside the > EXPECT_DCHECK_DEATH macro [1], but I never said that we shouldn't execute > \|statement\| when !DCHECK_IS_ON(). Pros/cons: > > We should always run \|statement\|: > - To be consistent with EXPECT_DEBUG_DEATH() [2]. > - To be able to verify the state after a DCHECK failure. For example, I would > like to verify that BLOCK_SHUTDOWN tasks posted after shutdown don't run in a > !DCHECK_IS_ON() build. > > We should only run \|statement\| when DCHECK_IS_ON(): > - There is no need to verify what happens after a DCHECK because the style guide > says that a DCHECK verifies a condition that "must always be true". The state > after a DCHECK might be invalid. > > Note that existing SchedulerLock tests pass regardless of whether \|statement\| is > executed. Hmm, this is surprising, that probably at the very least makes them flaky. The DCHECK are catching potential for race conditions, by running this code we run racy code which is probably undeterministically flaky :-(. I'd say testing what happens after a DCHECK doesn't make sense (i.e. +1 to your second point). Show quoted text > > [1] > https://codereview.chromium.org/1706123002/diff/400001/base/task_scheduler/sc... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
17 #endif	17 #endif

18	18

19 #endif // BASE_TASK_SCHEDULER_TEST_UTILS_H_	19 #endif // BASE_TASK_SCHEDULER_TEST_UTILS_H_

OLD	NEW