|
|
|
Created:
17 years, 5 months ago by DO NOT USE Modified:
16 years, 5 months ago Reviewers:
wan Base URL:
http://googletest.googlecode.com/svn/trunk/ Visibility:
Public. |
Add the functionality to disable a test at runtime.
unit test included.
Patch Set 1 #Patch Set 2 : Slightly improved the unit test #Patch Set 3 : Third revision #Patch Set 4 : Fourth version #
Total messages: 3
|
DO NOT USE
|
17 years, 5 months ago (2008年07月23日 18:49:17 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||
Hi, M-A, Please see my comments below. Thanks! http://codereview.appspot.com/2611/diff/5/7 File include/gtest/gtest.h (right): http://codereview.appspot.com/2611/diff/5/7#newcode229 Line 229: // Returns true if the current test has been dynamically disabled. Why do you emphasize "dynamically" here? Won't the function return true too when the test is statically disabled? http://codereview.appspot.com/2611/diff/5/7#newcode275 Line 275: void DisableThisTest(); This function needs to be static such that it can be called in subroutines that are not members of the test fixture. http://codereview.appspot.com/2611/diff/5/8 File src/gtest.cc (right): http://codereview.appspot.com/2611/diff/5/8#newcode1687 Line 1687: void Test::DisableThisTest() { Style nit: add a comment header on what this does. Also, group this with IsDisabled()? http://codereview.appspot.com/2611/diff/5/8#newcode1689 Line 1689: set_is_disabled(true); We need to make this thread-safe, as the user may call this from multiple threads concurrently. You can use the synchronization primitives in include/gtest/internal/gtest-port.h. They have a dummy implementation for now but will have a real implementation later. http://codereview.appspot.com/2611/diff/5/8#newcode1810 Line 1810: // We will run the test only if SetUp() had no fatal failure. Update this comment. http://codereview.appspot.com/2611/diff/5/8#newcode1811 Line 1811: if (!HasFatalFailure() && !Test::IsDisabled()) { The two calls should have the same style: use Test:: consistently. http://codereview.appspot.com/2611/diff/5/8#newcode1836 Line 1836: // We will run the test only if SetUp() was successful. The same. http://codereview.appspot.com/2611/diff/5/8#newcode1837 Line 1837: if (!HasFatalFailure() && !Test::IsDisabled()) { The same. http://codereview.appspot.com/2611/diff/5/8#newcode1856 Line 1856: bool Test::IsDisabled() { Add a comment header. http://codereview.appspot.com/2611/diff/5/8#newcode1858 Line 1858: is_disabled(); We should make reading is_disabled() thread-safe. http://codereview.appspot.com/2611/diff/5/8#newcode2006 Line 2006: // Runs the test only if the constructor of the test fixture didn't Update this. http://codereview.appspot.com/2611/diff/5/6 File test/gtest_unittest.cc (right): http://codereview.appspot.com/2611/diff/5/6#newcode108 Line 108: TEST_F(TestDisabledInConstructor, DoesntRun) { We should make sure the test's failures (if any) are ignored if the test is disabled. Therefore this won't tell us that the test body isn't run. http://codereview.appspot.com/2611/diff/5/6#newcode126 Line 126: } Need to test: - calling DisableThisTest() and IsDisabled() from a function that's not a member of the test fixture (nor the TEST itself). - calling DisableThisTest() in the test ctor prevents SetUp(), the test body, and TearDown() to be run. - calling DisableThisTest() in SetUp() prevents the test body to be run, but TearDown() should still be run. - both statically disabled tests and dynamically disabled tests are included in the count printed at the end of the test program. - test failures before calling DisableThisTest() are ignored. - test failures after calling DisableThisTest() are ignored. - Google Test should print the names of the dynamically disabled tests in the end. - calling DisableThisTest() and IsDisabled() from multiple threads concurrently is OK. For this you need to modify gtest_stress_test.cc.
Will send a new patch with the changes done. Some unit tests are still missing but the rest has been fixed. http://codereview.appspot.com/2611/diff/5/6 File test/gtest_unittest.cc (right): http://codereview.appspot.com/2611/diff/5/6#newcode108 Line 108: TEST_F(TestDisabledInConstructor, DoesntRun) { On 2008年07月25日 04:48:48, Zhanyong wrote: > We should make sure the test's failures (if any) are ignored if the test is > disabled. > > Therefore this won't tell us that the test body isn't run. I clarified the expectations in DisableThisTest(), so testing this is unnecessary, i.e. the test will fail if the test body is run. http://codereview.appspot.com/2611/diff/5/6#newcode126 Line 126: } On 2008年07月25日 04:48:48, Zhanyong wrote: > Need to test: > > - calling DisableThisTest() and IsDisabled() from a function that's not a member > of the test fixture (nor the TEST itself). Not done. I need to find out how to do that. > - calling DisableThisTest() in the test ctor prevents SetUp(), the test body, > and TearDown() to be run. Done. > - calling DisableThisTest() in SetUp() prevents the test body to be run, but > TearDown() should still be run. Done. > - both statically disabled tests and dynamically disabled tests are included in > the count printed at the end of the test program. Not done. I need to find out how to do that. > - test failures before calling DisableThisTest() are ignored. N/A > - test failures after calling DisableThisTest() are ignored. N/A > - Google Test should print the names of the dynamically disabled tests in the > end. Not done. I don't think this is in the scope of this change. > - calling DisableThisTest() and IsDisabled() from multiple threads concurrently > is OK. For this you need to modify gtest_stress_test.cc. Not done. There is no vcproj that includes this source file.