Destroy installed environments in normal code, not in static teardown.

Destruction in static teardown causes issues for Environments which own threads and try to join them in their destruction.

This may be a breaking change for users who call RUN_ALL_TESTS multiple times in the same main function if they also install environments, or those who access registered environments after RUN_ALL_TESTS.

The easiest fix is to only call RUN_ALL_TESTS once as the last line of the main function. Another potential fix is to re-register new instances of the Environment once before each call to RUN_ALL_TESTS.

PiperOrigin-RevId: 604800795
Change-Id: I37c44d4aca4a238052649f45a4b6b9cfb5355b71

4 files changed, 67 insertions, 51 deletions

diff --git a/googletest/include/gtest/gtest.h b/googletest/include/gtest/gtest.h
index 45400fd..15d0539 100644
--- a/googletest/include/gtest/gtest.h
+++ b/googletest/include/gtest/gtest.h
@@ -2308,7 +2308,8 @@ TestInfo* RegisterTest(const char* test_suite_name, const char* test_name,
 // tests are successful, or 1 otherwise.
 //
 // RUN_ALL_TESTS() should be invoked after the command line has been
-// parsed by InitGoogleTest().
+// parsed by InitGoogleTest(). RUN_ALL_TESTS will tear down and delete any
+// installed environments and should only be called once per binary.
 //
 // This function was formerly a macro; thus, it is in the global
 // namespace and has an all-caps name.
diff --git a/googletest/src/gtest.cc b/googletest/src/gtest.cc
index 6d6ff1b..d25f5c4 100644
--- a/googletest/src/gtest.cc
+++ b/googletest/src/gtest.cc
@@ -536,7 +536,8 @@ void InsertSyntheticTestCase(const std::string& name, CodeLocation location,
   if (ignored.find(name) != ignored.end()) return;
 
   const char kMissingInstantiation[] =  //
-      " is defined via TEST_P, but never instantiated. None of the test cases "
+      " is defined via TEST_P, but never instantiated. None of the test "
+      "cases "
       "will run. Either no INSTANTIATE_TEST_SUITE_P is provided or the only "
       "ones provided expand to nothing."
       "\n\n"
@@ -615,10 +616,12 @@ void TypeParameterizedTestSuiteRegistry::CheckForInstantiations() {
         "\n\n"
         "Ideally, TYPED_TEST_P definitions should only ever be included as "
         "part of binaries that intend to use them. (As opposed to, for "
-        "example, being placed in a library that may be linked in to get other "
+        "example, being placed in a library that may be linked in to get "
+        "other "
         "utilities.)"
         "\n\n"
-        "To suppress this error for this test suite, insert the following line "
+        "To suppress this error for this test suite, insert the following "
+        "line "
         "(in a non-header) in the namespace it is defined in:"
         "\n\n"
         "GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(" +
@@ -5991,6 +5994,12 @@ bool UnitTestImpl::RunAllTests() {
   }
 
   repeater->OnTestProgramEnd(*parent_);
+  // Destroy environments in normal code, not in static teardown.
+  bool delete_environment_on_teardown = true;
+  if (delete_environment_on_teardown) {
+    ForEach(environments_, internal::Delete<Environment>);
+    environments_.clear();
+  }
 
   if (!gtest_is_initialized_before_run_all_tests) {
     ColoredPrintf(
diff --git a/googletest/test/gtest_environment_test.cc b/googletest/test/gtest_environment_test.cc
index 122eaf3..03657c7 100644
--- a/googletest/test/gtest_environment_test.cc
+++ b/googletest/test/gtest_environment_test.cc
@@ -40,16 +40,21 @@ namespace {
 
 enum FailureType { NO_FAILURE, NON_FATAL_FAILURE, FATAL_FAILURE };
 
+// Was SetUp run?
+bool set_up_was_run;
+// Was TearDown run?
+bool tear_down_was_run;
+// Was the TEST run?
+bool test_was_run;
+
 // For testing using global test environments.
 class MyEnvironment : public testing::Environment {
  public:
-  MyEnvironment() { Reset(); }
-
   // Depending on the value of failure_in_set_up_, SetUp() will
   // generate a non-fatal failure, generate a fatal failure, or
   // succeed.
   void SetUp() override {
-    set_up_was_run_ = true;
+    set_up_was_run = true;
 
     switch (failure_in_set_up_) {
       case NON_FATAL_FAILURE:
@@ -65,36 +70,18 @@ class MyEnvironment : public testing::Environment {
 
   // Generates a non-fatal failure.
   void TearDown() override {
-    tear_down_was_run_ = true;
+    tear_down_was_run = true;
     ADD_FAILURE() << "Expected non-fatal failure in global tear-down.";
   }
 
-  // Resets the state of the environment s.t. it can be reused.
-  void Reset() {
-    failure_in_set_up_ = NO_FAILURE;
-    set_up_was_run_ = false;
-    tear_down_was_run_ = false;
-  }
-
   // We call this function to set the type of failure SetUp() should
   // generate.
   void set_failure_in_set_up(FailureType type) { failure_in_set_up_ = type; }
 
-  // Was SetUp() run?
-  bool set_up_was_run() const { return set_up_was_run_; }
-
-  // Was TearDown() run?
-  bool tear_down_was_run() const { return tear_down_was_run_; }
-
  private:
   FailureType failure_in_set_up_;
-  bool set_up_was_run_;
-  bool tear_down_was_run_;
 };
 
-// Was the TEST run?
-bool test_was_run;
-
 // The sole purpose of this TEST is to enable us to check whether it
 // was run.
 TEST(FooTest, Bar) { test_was_run = true; }
@@ -112,67 +99,88 @@ void Check(bool condition, const char* msg) {
 // The 'failure' parameter specifies the type of failure that should
 // be generated by the global set-up.
 int RunAllTests(MyEnvironment* env, FailureType failure) {
-  env->Reset();
-  env->set_failure_in_set_up(failure);
+  set_up_was_run = false;
+  tear_down_was_run = false;
   test_was_run = false;
+  env->set_failure_in_set_up(failure);
   testing::internal::GetUnitTestImpl()->ClearAdHocTestResult();
   return RUN_ALL_TESTS();
 }
 
-}  // namespace
-
-int main(int argc, char** argv) {
-  testing::InitGoogleTest(&argc, argv);
-
-  // Registers a global test environment, and verifies that the
-  // registration function returns its argument.
+// Registers a global test environment, and verifies that the
+// registration function returns its argument.
+MyEnvironment* RegisterTestEnv() {
   MyEnvironment* const env = new MyEnvironment;
   Check(testing::AddGlobalTestEnvironment(env) == env,
         "AddGlobalTestEnvironment() should return its argument.");
+  return env;
+}
 
-  // Verifies that RUN_ALL_TESTS() runs the tests when the global
-  // set-up is successful.
+// Verifies that RUN_ALL_TESTS() runs the tests when the global
+// set-up is successful.
+void TestGlobalSetUp() {
+  MyEnvironment* const env = RegisterTestEnv();
   Check(RunAllTests(env, NO_FAILURE) != 0,
         "RUN_ALL_TESTS() should return non-zero, as the global tear-down "
         "should generate a failure.");
   Check(test_was_run,
         "The tests should run, as the global set-up should generate no "
         "failure");
-  Check(env->tear_down_was_run(),
+  Check(tear_down_was_run,
         "The global tear-down should run, as the global set-up was run.");
+}
 
-  // Verifies that RUN_ALL_TESTS() runs the tests when the global
-  // set-up generates no fatal failure.
+// Verifies that RUN_ALL_TESTS() runs the tests when the global
+// set-up generates no fatal failure.
+void TestTestsRun() {
+  MyEnvironment* const env = RegisterTestEnv();
   Check(RunAllTests(env, NON_FATAL_FAILURE) != 0,
         "RUN_ALL_TESTS() should return non-zero, as both the global set-up "
         "and the global tear-down should generate a non-fatal failure.");
   Check(test_was_run,
         "The tests should run, as the global set-up should generate no "
         "fatal failure.");
-  Check(env->tear_down_was_run(),
+  Check(tear_down_was_run,
         "The global tear-down should run, as the global set-up was run.");
+}
 
-  // Verifies that RUN_ALL_TESTS() runs no test when the global set-up
-  // generates a fatal failure.
+// Verifies that RUN_ALL_TESTS() runs no test when the global set-up
+// generates a fatal failure.
+void TestNoTestsRunSetUpFailure() {
+  MyEnvironment* const env = RegisterTestEnv();
   Check(RunAllTests(env, FATAL_FAILURE) != 0,
         "RUN_ALL_TESTS() should return non-zero, as the global set-up "
         "should generate a fatal failure.");
   Check(!test_was_run,
         "The tests should not run, as the global set-up should generate "
         "a fatal failure.");
-  Check(env->tear_down_was_run(),
+  Check(tear_down_was_run,
         "The global tear-down should run, as the global set-up was run.");
+}
 
-  // Verifies that RUN_ALL_TESTS() doesn't do global set-up or
-  // tear-down when there is no test to run.
+// Verifies that RUN_ALL_TESTS() doesn't do global set-up or
+// tear-down when there is no test to run.
+void TestNoTestsSkipsSetUp() {
+  MyEnvironment* const env = RegisterTestEnv();
   GTEST_FLAG_SET(filter, "-*");
   Check(RunAllTests(env, NO_FAILURE) == 0,
         "RUN_ALL_TESTS() should return zero, as there is no test to run.");
-  Check(!env->set_up_was_run(),
+  Check(!set_up_was_run,
         "The global set-up should not run, as there is no test to run.");
-  Check(!env->tear_down_was_run(),
+  Check(!tear_down_was_run,
         "The global tear-down should not run, "
         "as the global set-up was not run.");
+}
+
+}  // namespace
+
+int main(int argc, char** argv) {
+  testing::InitGoogleTest(&argc, argv);
+
+  TestGlobalSetUp();
+  TestTestsRun();
+  TestNoTestsRunSetUpFailure();
+  TestNoTestsSkipsSetUp();
 
   printf("PASS\n");
   return 0;
diff --git a/googletest/test/gtest_repeat_test.cc b/googletest/test/gtest_repeat_test.cc
index f67b788..55103d0 100644
--- a/googletest/test/gtest_repeat_test.cc
+++ b/googletest/test/gtest_repeat_test.cc
@@ -61,7 +61,6 @@ int g_environment_tear_down_count = 0;
 
 class MyEnvironment : public testing::Environment {
  public:
-  MyEnvironment() = default;
   void SetUp() override { g_environment_set_up_count++; }
   void TearDown() override { g_environment_tear_down_count++; }
 };
@@ -117,6 +116,7 @@ void ResetCounts() {
   g_should_pass_count = 0;
   g_death_test_count = 0;
   g_param_test_count = 0;
+  testing::AddGlobalTestEnvironment(new MyEnvironment);
 }
 
 // Checks that the count for each test is expected.
@@ -197,8 +197,6 @@ void TestRepeatWithFilterForFailedTests(int repeat) {
 int main(int argc, char **argv) {
   testing::InitGoogleTest(&argc, argv);
 
-  testing::AddGlobalTestEnvironment(new MyEnvironment);
-
   TestRepeatUnspecified();
   TestRepeat(0);
   TestRepeat(1);