Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 1 Apr 2019 18:07:48 +0000 (UTC)
From:      Enji Cooper <ngie@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org
Subject:   svn commit: r345770 - in head: contrib/googletest/googletest contrib/googletest/googletest/docs contrib/googletest/googletest/src contrib/googletest/googletest/test lib/googletest/gtest_main/tests
Message-ID:  <201904011807.x31I7mKF062717@repo.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: ngie
Date: Mon Apr  1 18:07:48 2019
New Revision: 345770
URL: https://svnweb.freebsd.org/changeset/base/345770

Log:
  Import proof-of-concept for handling `GTEST_SKIP()` in `Environment::SetUp`
  
  Per the upstream pull-request [1]:
  
  ```
    gtest prior to this change would completely ignore `GTEST_SKIP()` if
    called in `Environment::SetUp()`, instead of bailing out early, unlike
    `Test::SetUp()`, which would cause the tests themselves to be skipped.
    The only way (prior to this change) to skip the tests would be to
    trigger a fatal error via `GTEST_FAIL()`.
  
    Desirable behavior, in this case, when dealing with
    `Environment::SetUp()` is to check for prerequisites on a system
    (example, kernel supports a particular featureset, e.g., capsicum), and
    skip the tests. The alternatives prior to this change would be
    undesirable:
  
    - Failing sends the wrong message to the test user, as the result of the
      tests is indeterminate, not failed.
    - Having to add per-test class abstractions that override `SetUp()` to
      test for the capsicum feature set, then skip all of the tests in their
      respective SetUp fixtures, would be a lot of human and computational
      work; checking for the feature would need to be done for all of the
      tests, instead of once for all of the tests.
  
    For those reasons, making `Environment::SetUp()` handle `GTEST_SKIP()`,
    by not executing the testcases, is the most desirable solution.
  
    In order to properly diagnose what happened when running the tests if
    they are skipped, print out the diagnostics in an ad hoc manner.
  
    Update the documentation to note this change and integrate a new test,
    gtest_skip_in_environment_setup_test, into the test suite.
  
    This change addresses #2189.
  
    Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>
  ```
  
  The goal with my merging in this change is to avoid requiring extensive
  refactoring/retesting of test suites when ensuring prerequisites are met,
  e.g., checking for a CAPABILITIES-enabled kernel before running capsicum-test
  (see D19758 for more details).
  
  The proof-of-concept is being imported before accepted by the upstream
  project due to the fact that the upstream project is undergoing a potential
  development freeze and the maintainers aren't responding to my PR.
  
  1. https://github.com/google/googletest/pull/2203
  
  Reported by:	asomers (https://github.com/google/googletest/issues/2189)
  Reviewed by:	asomers
  Approved by:	emaste (mentor)
  MFC after:	2 months
  Differential Revision:	https://reviews.freebsd.org/D19765

Added:
  head/contrib/googletest/googletest/test/gtest_skip_in_environment_setup_test.cc   (contents, props changed)
Modified:
  head/contrib/googletest/googletest/CMakeLists.txt
  head/contrib/googletest/googletest/Makefile.am
  head/contrib/googletest/googletest/docs/advanced.md
  head/contrib/googletest/googletest/src/gtest.cc
  head/contrib/googletest/googletest/test/BUILD.bazel
  head/lib/googletest/gtest_main/tests/Makefile

Modified: head/contrib/googletest/googletest/CMakeLists.txt
==============================================================================
--- head/contrib/googletest/googletest/CMakeLists.txt	Mon Apr  1 17:44:20 2019	(r345769)
+++ head/contrib/googletest/googletest/CMakeLists.txt	Mon Apr  1 18:07:48 2019	(r345770)
@@ -217,6 +217,7 @@ if (gtest_build_tests)
     test/gtest-typed-test2_test.cc)
   cxx_test(gtest_unittest gtest_main)
   cxx_test(gtest-unittest-api_test gtest)
+  cxx_test(gtest_skip_in_environment_setup_test gtest_main)
   cxx_test(gtest_skip_test gtest_main)
 
   ############################################################

Modified: head/contrib/googletest/googletest/Makefile.am
==============================================================================
--- head/contrib/googletest/googletest/Makefile.am	Mon Apr  1 17:44:20 2019	(r345769)
+++ head/contrib/googletest/googletest/Makefile.am	Mon Apr  1 18:07:48 2019	(r345770)
@@ -290,6 +290,12 @@ test_gtest_all_test_SOURCES = test/gtest_all_test.cc
 test_gtest_all_test_LDADD = lib/libgtest_main.la \
                             lib/libgtest.la
 
+TESTS += test/gtest_skip_in_environment_setup_test
+check_PROGRAMS += test/gtest_skip_in_environment_setup_test
+test_gtest_skip_in_environment_setup_test_SOURCES = test/gtest_skip_in_environment_setup_test.cc
+test_gtest_skip_in_environment_setup_test_LDADD= lib/libgtest_main.la \
+                                                 lib/libgtest.la
+
 # Tests that fused gtest files compile and work.
 FUSED_GTEST_SRC = \
   fused-src/gtest/gtest-all.cc \

Modified: head/contrib/googletest/googletest/docs/advanced.md
==============================================================================
--- head/contrib/googletest/googletest/docs/advanced.md	Mon Apr  1 17:44:20 2019	(r345769)
+++ head/contrib/googletest/googletest/docs/advanced.md	Mon Apr  1 18:07:48 2019	(r345770)
@@ -1289,8 +1289,10 @@ Environment* AddGlobalTestEnvironment(Environment* env
 ```
 
 Now, when `RUN_ALL_TESTS()` is called, it first calls the `SetUp()` method of
-the environment object, then runs the tests if there was no fatal failures, and
-finally calls `TearDown()` of the environment object.
+each environment object, then runs the tests if none of the environments
+reported fatal failures and `GTEST_SKIP()` was not called. `RUN_ALL_TESTS()`
+always calls `TearDown()` with each environment object, regardless of whether
+or not the tests were run.
 
 It's OK to register multiple environment objects. In this case, their `SetUp()`
 will be called in the order they are registered, and their `TearDown()` will be

Modified: head/contrib/googletest/googletest/src/gtest.cc
==============================================================================
--- head/contrib/googletest/googletest/src/gtest.cc	Mon Apr  1 17:44:20 2019	(r345769)
+++ head/contrib/googletest/googletest/src/gtest.cc	Mon Apr  1 18:07:48 2019	(r345770)
@@ -5243,9 +5243,23 @@ bool UnitTestImpl::RunAllTests() {
       ForEach(environments_, SetUpEnvironment);
       repeater->OnEnvironmentsSetUpEnd(*parent_);
 
-      // Runs the tests only if there was no fatal failure during global
-      // set-up.
-      if (!Test::HasFatalFailure()) {
+      // Runs the tests only if there was no fatal failure or skip triggered
+      // during global set-up.
+      if (Test::IsSkipped()) {
+        // Emit diagnostics when global set-up calls skip, as it will not be
+        // emitted by default.
+        TestResult& test_result =
+          *internal::GetUnitTestImpl()->current_test_result();
+        for (int j = 0; j < test_result.total_part_count(); ++j) {
+          const TestPartResult& test_part_result =
+            test_result.GetTestPartResult(j);
+          if (test_part_result.type() == TestPartResult::kSkip) {
+            const std::string& result = test_part_result.message();
+            printf("%s\n", result.c_str());
+          }
+        }
+        fflush(stdout);
+      } else if (!Test::HasFatalFailure()) {
         for (int test_index = 0; test_index < total_test_case_count();
              test_index++) {
           GetMutableTestCase(test_index)->Run();

Modified: head/contrib/googletest/googletest/test/BUILD.bazel
==============================================================================
--- head/contrib/googletest/googletest/test/BUILD.bazel	Mon Apr  1 17:44:20 2019	(r345769)
+++ head/contrib/googletest/googletest/test/BUILD.bazel	Mon Apr  1 18:07:48 2019	(r345770)
@@ -311,6 +311,13 @@ cc_binary(
     deps = ["//:gtest"],
 )
 
+cc_test(
+    name = "gtest_skip_in_environment_setup_test",
+    size = "small",
+    srcs = ["gtest_skip_in_environment_setup_test.cc"],
+    deps = ["//:gtest_main"],
+)
+
 py_test(
     name = "googletest-list-tests-unittest",
     size = "small",

Added: head/contrib/googletest/googletest/test/gtest_skip_in_environment_setup_test.cc
==============================================================================
--- /dev/null	00:00:00 1970	(empty, because file is newly added)
+++ head/contrib/googletest/googletest/test/gtest_skip_in_environment_setup_test.cc	Mon Apr  1 18:07:48 2019	(r345770)
@@ -0,0 +1,60 @@
+// Copyright 2019, Google Inc.
+// All rights reserved.
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+//
+// This test verifies that skipping in the environment results in the
+// testcases being skipped.
+//
+// This is a reproduction case for
+// https://github.com/google/googletest/issues/2189 .
+
+#include <iostream>
+#include <gtest/gtest.h>
+
+class SetupEnvironment : public testing::Environment {
+public:
+  void SetUp() override {
+    GTEST_SKIP() << "Skipping the entire environment";
+  }
+};
+
+TEST(Test, AlwaysPasses) {
+  EXPECT_EQ(true, true);
+}
+
+TEST(Test, AlwaysFails) {
+  EXPECT_EQ(true, false);
+}
+
+int main(int argc, char **argv) {
+  testing::InitGoogleTest(&argc, argv);
+
+  testing::AddGlobalTestEnvironment(new SetupEnvironment());
+
+  return (RUN_ALL_TESTS());
+}

Modified: head/lib/googletest/gtest_main/tests/Makefile
==============================================================================
--- head/lib/googletest/gtest_main/tests/Makefile	Mon Apr  1 17:44:20 2019	(r345769)
+++ head/lib/googletest/gtest_main/tests/Makefile	Mon Apr  1 18:07:48 2019	(r345770)
@@ -19,6 +19,7 @@ GTESTS+=	gtest_sole_header_test
 GTESTS+=	googletest-test-part-test
 GTESTS+=	gtest-typed-test_test
 GTESTS+=	gtest_skip_test
+GTESTS+=	gtest_skip_in_environment_setup_test
 GTESTS+=	gtest_unittest
 
 CXXFLAGS+=	-I${GOOGLETEST_SRCROOT}/include



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201904011807.x31I7mKF062717>