From 019d19af978f05b774407e0d46a3bda2c18c67c6 Mon Sep 17 00:00:00 2001 From: shiqian Date: Fri, 12 Sep 2008 04:01:37 +0000 Subject: Improves thread-safe death tests by changing to the original working directory before they are executed; also fixes out-dated comments about death tests. --- include/gtest/gtest-death-test.h | 36 +++++++------ include/gtest/gtest.h | 4 ++ include/gtest/internal/gtest-filepath.h | 6 +++ src/gtest-death-test.cc | 23 ++++++++- src/gtest-filepath.cc | 18 +++++++ src/gtest-internal-inl.h | 21 ++++++++ src/gtest.cc | 6 +++ test/gtest-death-test_test.cc | 90 +++++++++++++++++++++++---------- test/gtest-filepath_test.cc | 32 ++++++++++++ test/gtest_unittest.cc | 8 ++- 10 files changed, 198 insertions(+), 46 deletions(-) diff --git a/include/gtest/gtest-death-test.h b/include/gtest/gtest-death-test.h index cbd41fe6..6fae47fb 100644 --- a/include/gtest/gtest-death-test.h +++ b/include/gtest/gtest-death-test.h @@ -56,29 +56,19 @@ GTEST_DECLARE_string(death_test_style); // Here's what happens when an ASSERT_DEATH* or EXPECT_DEATH* is // executed: // -// 1. The assertion fails immediately if there are more than one -// active threads. This is because it's safe to fork() only when -// there is a single thread. +// 1. It generates a warning if there is more than one active +// thread. This is because it's safe to fork() or clone() only +// when there is a single thread. // -// 2. The parent process forks a sub-process and runs the death test -// in it; the sub-process exits with code 0 at the end of the death -// test, if it hasn't exited already. +// 2. The parent process clone()s a sub-process and runs the death +// test in it; the sub-process exits with code 0 at the end of the +// death test, if it hasn't exited already. // // 3. The parent process waits for the sub-process to terminate. // // 4. The parent process checks the exit code and error message of // the sub-process. // -// Note: -// -// It's not safe to call exit() if the current process is forked from -// a multi-threaded process, so people usually call _exit() instead in -// such a case. However, we are not concerned with this as we run -// death tests only when there is a single thread. Since exit() has a -// cleaner semantics (it also calls functions registered with atexit() -// and on_exit()), this macro calls exit() instead of _exit() to -// terminate the child process. -// // Examples: // // ASSERT_DEATH(server.SendMessage(56, "Hello"), "Invalid port number"); @@ -95,6 +85,20 @@ GTEST_DECLARE_string(death_test_style); // } // // ASSERT_EXIT(client.HangUpServer(), KilledBySIGHUP, "Hanging up!"); +// +// Known caveats: +// +// A "threadsafe" style death test obtains the path to the test +// program from argv[0] and re-executes it in the sub-process. For +// simplicity, the current implementation doesn't search the PATH +// when launching the sub-process. This means that the user must +// invoke the test program via a path that contains at least one +// path separator (e.g. path/to/foo_test and +// /absolute/path/to/bar_test are fine, but foo_test is not). This +// is rarely a problem as people usually don't put the test binary +// directory in PATH. +// +// TODO(wan@google.com): make thread-safe death tests search the PATH. // Asserts that a given statement causes the program to exit, with an // integer exit status that satisfies predicate, and emitting error output diff --git a/include/gtest/gtest.h b/include/gtest/gtest.h index 37ea6b04..057efa26 100644 --- a/include/gtest/gtest.h +++ b/include/gtest/gtest.h @@ -479,6 +479,10 @@ class UnitTest { // INTERNAL IMPLEMENTATION - DO NOT USE IN A USER PROGRAM. int Run() GTEST_MUST_USE_RESULT; + // Returns the working directory when the first TEST() or TEST_F() + // was executed. The UnitTest object owns the string. + const char* original_working_dir() const; + // Returns the TestCase object for the test that's currently running, // or NULL if no test is running. const TestCase* current_test_case() const; diff --git a/include/gtest/internal/gtest-filepath.h b/include/gtest/internal/gtest-filepath.h index 308a2c64..fecdddcc 100644 --- a/include/gtest/internal/gtest-filepath.h +++ b/include/gtest/internal/gtest-filepath.h @@ -70,6 +70,9 @@ class FilePath { String ToString() const { return pathname_; } const char* c_str() const { return pathname_.c_str(); } + // Returns the current working directory, or "" if unsuccessful. + static FilePath GetCurrentDir(); + // Given directory = "dir", base_name = "test", number = 0, // extension = "xml", returns "dir/test.xml". If number is greater // than zero (e.g., 12), returns "dir/test_12.xml". @@ -91,6 +94,9 @@ class FilePath { const FilePath& base_name, const char* extension); + // Returns true iff the path is NULL or "". + bool IsEmpty() const { return c_str() == NULL || *c_str() == '\0'; } + // If input name has a trailing separator character, removes it and returns // the name, otherwise return the name string unmodified. // On Windows platform, uses \ as the separator, other platforms use /. diff --git a/src/gtest-death-test.cc b/src/gtest-death-test.cc index cb0d3cd7..971c3005 100644 --- a/src/gtest-death-test.cc +++ b/src/gtest-death-test.cc @@ -546,11 +546,32 @@ struct ExecDeathTestArgs { }; // The main function for a threadsafe-style death test child process. +// This function is called in a clone()-ed process and thus must avoid +// any potentially unsafe operations like malloc or libc functions. static int ExecDeathTestChildMain(void* child_arg) { ExecDeathTestArgs* const args = static_cast(child_arg); GTEST_DEATH_TEST_CHECK_SYSCALL(close(args->close_fd)); + + // We need to execute the test program in the same environment where + // it was originally invoked. Therefore we change to the original + // working directory first. + const char* const original_dir = + UnitTest::GetInstance()->original_working_dir(); + // We can safely call chdir() as it's a direct system call. + if (chdir(original_dir) != 0) { + DeathTestAbort("chdir(\"%s\") failed: %s", + original_dir, strerror(errno)); + return EXIT_FAILURE; + } + + // We can safely call execve() as it's a direct system call. We + // cannot use execvp() as it's a libc function and thus potentially + // unsafe. Since execve() doesn't search the PATH, the user must + // invoke the test program via a valid path that contains at least + // one path separator. execve(args->argv[0], args->argv, environ); - DeathTestAbort("execve failed: %s", strerror(errno)); + DeathTestAbort("execve(%s, ...) in %s failed: %s", + args->argv[0], original_dir, strerror(errno)); return EXIT_FAILURE; } diff --git a/src/gtest-filepath.cc b/src/gtest-filepath.cc index 3c32c705..2a5be8ce 100644 --- a/src/gtest-filepath.cc +++ b/src/gtest-filepath.cc @@ -32,6 +32,8 @@ #include #include +#include + #ifdef _WIN32_WCE #include #elif defined(_WIN32) @@ -40,6 +42,7 @@ #include #else #include +#include #endif // _WIN32_WCE or _WIN32 #include @@ -66,6 +69,21 @@ const char kPathSeparatorString[] = "/"; const char kCurrentDirectoryString[] = "./"; #endif // GTEST_OS_WINDOWS +// Returns the current working directory, or "" if unsuccessful. +FilePath FilePath::GetCurrentDir() { +#ifdef _WIN32_WCE +// Windows CE doesn't have a current directory, so we just return +// something reasonable. + return FilePath(kCurrentDirectoryString); +#elif defined(GTEST_OS_WINDOWS) + char cwd[_MAX_PATH + 1] = {}; + return FilePath(_getcwd(cwd, sizeof(cwd)) == NULL ? "" : cwd); +#else + char cwd[PATH_MAX + 1] = {}; + return FilePath(getcwd(cwd, sizeof(cwd)) == NULL ? "" : cwd); +#endif +} + // Returns a copy of the FilePath with the case-insensitive extension removed. // Example: FilePath("dir/file.exe").RemoveExtension("EXE") returns // FilePath("dir/file"). If a case-insensitive extension is not diff --git a/src/gtest-internal-inl.h b/src/gtest-internal-inl.h index 6aafc7bf..d4889483 100644 --- a/src/gtest-internal-inl.h +++ b/src/gtest-internal-inl.h @@ -1003,6 +1003,21 @@ class UnitTestImpl : public TestPartResultReporterInterface { void AddTestInfo(Test::SetUpTestCaseFunc set_up_tc, Test::TearDownTestCaseFunc tear_down_tc, TestInfo * test_info) { + // In order to support thread-safe death tests, we need to + // remember the original working directory when the test program + // was first invoked. We cannot do this in RUN_ALL_TESTS(), as + // the user may have changed the current directory before calling + // RUN_ALL_TESTS(). Therefore we capture the current directory in + // AddTestInfo(), which is called to register a TEST or TEST_F + // before main() is reached. + if (original_working_dir_.IsEmpty()) { + original_working_dir_.Set(FilePath::GetCurrentDir()); + if (original_working_dir_.IsEmpty()) { + printf("%s\n", "Failed to get the current working directory."); + abort(); + } + } + GetTestCase(test_info->test_case_name(), test_info->test_case_comment(), set_up_tc, @@ -1083,9 +1098,15 @@ class UnitTestImpl : public TestPartResultReporterInterface { #endif // GTEST_HAS_DEATH_TEST private: + friend class ::testing::UnitTest; + // The UnitTest object that owns this implementation object. UnitTest* const parent_; + // The working directory when the first TEST() or TEST_F() was + // executed. + internal::FilePath original_working_dir_; + // Points to (but doesn't own) the test part result reporter. TestPartResultReporterInterface* test_part_result_reporter_; diff --git a/src/gtest.cc b/src/gtest.cc index 09f6bae4..8ca6ac8e 100644 --- a/src/gtest.cc +++ b/src/gtest.cc @@ -3211,6 +3211,12 @@ int UnitTest::Run() { #endif // GTEST_OS_WINDOWS } +// Returns the working directory when the first TEST() or TEST_F() was +// executed. +const char* UnitTest::original_working_dir() const { + return impl_->original_working_dir_.c_str(); +} + // Returns the TestCase object for the test that's currently running, // or NULL if no test is running. // L < mutex_ diff --git a/test/gtest-death-test_test.cc b/test/gtest-death-test_test.cc index 4bfd9d63..9d69b2cd 100644 --- a/test/gtest-death-test_test.cc +++ b/test/gtest-death-test_test.cc @@ -36,6 +36,8 @@ #ifdef GTEST_HAS_DEATH_TEST +#include +#include #include // Indicates that this translation unit is part of Google Test's @@ -85,13 +87,19 @@ class TestForDeathTest : public testing::Test { protected: // A static member function that's expected to die. static void StaticMemberFunction() { - GTEST_LOG(FATAL, "death inside StaticMemberFunction()."); + fprintf(stderr, "%s", "death inside StaticMemberFunction()."); + // We call _exit() instead of exit(), as the former is a direct + // system call and thus safer in the presence of threads. exit() + // will invoke user-defined exit-hooks, which may do dangerous + // things that conflict with death tests. + _exit(1); } // A method of the test fixture that may die. void MemberFunction() { if (should_die_) { - GTEST_LOG(FATAL, "death inside MemberFunction()."); + fprintf(stderr, "%s", "death inside MemberFunction()."); + _exit(1); } } @@ -157,13 +165,13 @@ int DieInDebugElse12(int* sideeffect) { return 12; } -// Returns the exit status of a process that calls exit(2) with a +// Returns the exit status of a process that calls _exit(2) with a // given exit code. This is a helper function for the // ExitStatusPredicateTest test suite. static int NormalExitStatus(int exit_code) { pid_t child_pid = fork(); if (child_pid == 0) { - exit(exit_code); + _exit(exit_code); } int status; waitpid(child_pid, &status, 0); @@ -179,7 +187,7 @@ static int KilledExitStatus(int signum) { pid_t child_pid = fork(); if (child_pid == 0) { raise(signum); - exit(1); + _exit(1); } int status; waitpid(child_pid, &status, 0); @@ -223,7 +231,7 @@ TEST_F(TestForDeathTest, SingleStatement) { ASSERT_DEATH(return, ""); if (true) - EXPECT_DEATH(exit(1), ""); + EXPECT_DEATH(_exit(1), ""); else // This empty "else" branch is meant to ensure that EXPECT_DEATH // doesn't expand into an "if" statement without an "else" @@ -235,12 +243,12 @@ TEST_F(TestForDeathTest, SingleStatement) { if (false) ; else - EXPECT_DEATH(exit(1), "") << 1 << 2 << 3; + EXPECT_DEATH(_exit(1), "") << 1 << 2 << 3; } void DieWithEmbeddedNul() { fprintf(stderr, "Hello%cworld.\n", '\0'); - abort(); + _exit(1); } // Tests that EXPECT_DEATH and ASSERT_DEATH work when the error @@ -257,24 +265,40 @@ TEST_F(TestForDeathTest, DISABLED_EmbeddedNulInMessage) { TEST_F(TestForDeathTest, SwitchStatement) { switch (0) default: - ASSERT_DEATH(exit(1), "") << "exit in default switch handler"; + ASSERT_DEATH(_exit(1), "") << "exit in default switch handler"; switch (0) case 0: - EXPECT_DEATH(exit(1), "") << "exit in switch case"; + EXPECT_DEATH(_exit(1), "") << "exit in switch case"; } -// Tests that a static member function can be used in a death test. -TEST_F(TestForDeathTest, StaticMemberFunction) { +// Tests that a static member function can be used in a "fast" style +// death test. +TEST_F(TestForDeathTest, StaticMemberFunctionFastStyle) { + testing::GTEST_FLAG(death_test_style) = "fast"; ASSERT_DEATH(StaticMemberFunction(), "death.*StaticMember"); } -// Tests that a method of the test fixture can be used in a death test. -TEST_F(TestForDeathTest, MemberFunction) { +// Tests that a method of the test fixture can be used in a "fast" +// style death test. +TEST_F(TestForDeathTest, MemberFunctionFastStyle) { + testing::GTEST_FLAG(death_test_style) = "fast"; should_die_ = true; EXPECT_DEATH(MemberFunction(), "inside.*MemberFunction"); } +// Tests that death tests work even if the current directory has been +// changed. +TEST_F(TestForDeathTest, FastDeathTestInChangedDir) { + testing::GTEST_FLAG(death_test_style) = "fast"; + + chdir("/"); + EXPECT_EXIT(_exit(1), testing::ExitedWithCode(1), ""); + + chdir("/"); + ASSERT_DEATH(_exit(1), ""); +} + // Repeats a representative sample of death tests in the "threadsafe" style: TEST_F(TestForDeathTest, StaticMemberFunctionThreadsafeStyle) { @@ -292,14 +316,24 @@ TEST_F(TestForDeathTest, ThreadsafeDeathTestInLoop) { testing::GTEST_FLAG(death_test_style) = "threadsafe"; for (int i = 0; i < 3; ++i) - EXPECT_EXIT(exit(i), testing::ExitedWithCode(i), "") << ": i = " << i; + EXPECT_EXIT(_exit(i), testing::ExitedWithCode(i), "") << ": i = " << i; +} + +TEST_F(TestForDeathTest, ThreadsafeDeathTestInChangedDir) { + testing::GTEST_FLAG(death_test_style) = "threadsafe"; + + chdir("/"); + EXPECT_EXIT(_exit(1), testing::ExitedWithCode(1), ""); + + chdir("/"); + ASSERT_DEATH(_exit(1), ""); } TEST_F(TestForDeathTest, MixedStyles) { testing::GTEST_FLAG(death_test_style) = "threadsafe"; - EXPECT_DEATH(exit(1), ""); + EXPECT_DEATH(_exit(1), ""); testing::GTEST_FLAG(death_test_style) = "fast"; - EXPECT_DEATH(exit(1), ""); + EXPECT_DEATH(_exit(1), ""); } namespace { @@ -316,7 +350,7 @@ TEST_F(TestForDeathTest, DoesNotExecuteAtforkHooks) { testing::GTEST_FLAG(death_test_style) = "threadsafe"; pthread_flag = false; ASSERT_EQ(0, pthread_atfork(&SetPthreadFlag, NULL, NULL)); - ASSERT_DEATH(exit(1), ""); + ASSERT_DEATH(_exit(1), ""); ASSERT_FALSE(pthread_flag); } @@ -528,8 +562,8 @@ TEST_F(TestForDeathTest, AssertDebugDeathAborts) { // Tests the *_EXIT family of macros, using a variety of predicates. TEST_F(TestForDeathTest, ExitMacros) { - EXPECT_EXIT(exit(1), testing::ExitedWithCode(1), ""); - ASSERT_EXIT(exit(42), testing::ExitedWithCode(42), ""); + EXPECT_EXIT(_exit(1), testing::ExitedWithCode(1), ""); + ASSERT_EXIT(_exit(42), testing::ExitedWithCode(42), ""); EXPECT_EXIT(raise(SIGKILL), testing::KilledBySignal(SIGKILL), "") << "foo"; ASSERT_EXIT(raise(SIGUSR2), testing::KilledBySignal(SIGUSR2), "") << "bar"; @@ -539,7 +573,7 @@ TEST_F(TestForDeathTest, ExitMacros) { }, "This failure is expected."); EXPECT_FATAL_FAILURE({ // NOLINT - ASSERT_EXIT(exit(0), testing::KilledBySignal(SIGSEGV), "") + ASSERT_EXIT(_exit(0), testing::KilledBySignal(SIGSEGV), "") << "This failure is expected, too."; }, "This failure is expected, too."); } @@ -547,7 +581,7 @@ TEST_F(TestForDeathTest, ExitMacros) { TEST_F(TestForDeathTest, InvalidStyle) { testing::GTEST_FLAG(death_test_style) = "rococo"; EXPECT_NONFATAL_FAILURE({ // NOLINT - EXPECT_DEATH(exit(0), "") << "This failure is expected."; + EXPECT_DEATH(_exit(0), "") << "This failure is expected."; }, "This failure is expected."); } @@ -794,7 +828,7 @@ TEST_F(MacroLogicDeathTest, ChildDoesNotDie) { // This time there are two calls to Abort: one since the test didn't // die, and another from the ReturnSentinel when it's destroyed. The // sentinel normally isn't destroyed if a test doesn't die, since - // exit(2) is called in that case by ForkingDeathTest, but not by + // _exit(2) is called in that case by ForkingDeathTest, but not by // our MockDeathTest. ASSERT_EQ(2, factory_->AbortCalls()); EXPECT_EQ(DeathTest::TEST_DID_NOT_DIE, @@ -813,18 +847,18 @@ static size_t GetSuccessfulTestPartCount() { // Tests that a successful death test does not register a successful // test part. TEST(SuccessRegistrationDeathTest, NoSuccessPart) { - EXPECT_DEATH(exit(1), ""); + EXPECT_DEATH(_exit(1), ""); EXPECT_EQ(0u, GetSuccessfulTestPartCount()); } TEST(StreamingAssertionsDeathTest, DeathTest) { - EXPECT_DEATH(exit(1), "") << "unexpected failure"; - ASSERT_DEATH(exit(1), "") << "unexpected failure"; + EXPECT_DEATH(_exit(1), "") << "unexpected failure"; + ASSERT_DEATH(_exit(1), "") << "unexpected failure"; EXPECT_NONFATAL_FAILURE({ // NOLINT - EXPECT_DEATH(exit(0), "") << "expected failure"; + EXPECT_DEATH(_exit(0), "") << "expected failure"; }, "expected failure"); EXPECT_FATAL_FAILURE({ // NOLINT - ASSERT_DEATH(exit(0), "") << "expected failure"; + ASSERT_DEATH(_exit(0), "") << "expected failure"; }, "expected failure"); } diff --git a/test/gtest-filepath_test.cc b/test/gtest-filepath_test.cc index f4b70c36..603427c1 100644 --- a/test/gtest-filepath_test.cc +++ b/test/gtest-filepath_test.cc @@ -91,6 +91,38 @@ const char* MakeTempDir() { } #endif // _WIN32_WCE +#ifndef _WIN32_WCE + +TEST(GetCurrentDirTest, ReturnsCurrentDir) { + EXPECT_FALSE(FilePath::GetCurrentDir().IsEmpty()); + +#ifdef GTEST_OS_WINDOWS + _chdir(PATH_SEP); + const FilePath cwd = FilePath::GetCurrentDir(); + // Skips the ":". + const char* const cwd_without_drive = strchr(cwd.c_str(), ':'); + ASSERT_TRUE(cwd_without_drive != NULL); + EXPECT_STREQ(PATH_SEP, cwd_without_drive + 1); +#else + chdir(PATH_SEP); + EXPECT_STREQ(PATH_SEP, FilePath::GetCurrentDir().c_str()); +#endif +} + +#endif // _WIN32_WCE + +TEST(IsEmptyTest, ReturnsTrueForEmptyPath) { + EXPECT_TRUE(FilePath("").IsEmpty()); + EXPECT_TRUE(FilePath(NULL).IsEmpty()); +} + +TEST(IsEmptyTest, ReturnsFalseForNonEmptyPath) { + EXPECT_FALSE(FilePath("a").IsEmpty()); + EXPECT_FALSE(FilePath(".").IsEmpty()); + EXPECT_FALSE(FilePath("a/b").IsEmpty()); + EXPECT_FALSE(FilePath("a\\b\\").IsEmpty()); +} + // FilePath's functions used by UnitTestOptions::GetOutputFile. // RemoveDirectoryName "" -> "" diff --git a/test/gtest_unittest.cc b/test/gtest_unittest.cc index 7c5123c2..8343795a 100644 --- a/test/gtest_unittest.cc +++ b/test/gtest_unittest.cc @@ -1142,7 +1142,8 @@ TEST(ParseInt32FlagTest, ParsesAndReturnsValidValue) { } // For the same reason we are not explicitly testing everything in the -// Test class, there are no separate tests for the following classes: +// Test class, there are no separate tests for the following classes +// (except for some trivial cases): // // TestCase, UnitTest, UnitTestResultPrinter. // @@ -1150,6 +1151,11 @@ TEST(ParseInt32FlagTest, ParsesAndReturnsValidValue) { // // TEST, TEST_F, RUN_ALL_TESTS +TEST(UnitTestTest, CanGetOriginalWorkingDir) { + ASSERT_TRUE(UnitTest::GetInstance()->original_working_dir() != NULL); + EXPECT_STRNE(UnitTest::GetInstance()->original_working_dir(), ""); +} + // This group of tests is for predicate assertions (ASSERT_PRED*, etc) // of various arities. They do not attempt to be exhaustive. Rather, // view them as smoke tests that can be easily reviewed and verified. -- cgit v1.2.3