Use GTEST_SKIP for SKIP

Summary:
The anticipated (and desired) effect of this change is to make it so that `SKIP` in `SetUp` will actually always skip the test. Right now, test behavior depends on the setting `FOLLY_SKIP_AS_FAILURE` — when this is false, the test will get **executed** despite being "skipped".

I believe it's unlikely that tests rely on the current behavior of doing `SKIP` from `SetUp` and running the test anyway. So, I expect this to be a relatively safe change.

Reviewed By: simpkins

Differential Revision: D15703771

fbshipit-source-id: d32e1d9c4cb8cb99c667882b81946154c6ca906b

folly/test/TestUtils.h

@@ -45,13 +45,39 @@
 // the test due to runtime issues or behavior that do not necessarily indicate
 // a problem with the code.
 //
-// googletest does not have a built-in mechanism to report tests as
-// skipped a run time.  We either report the test as successful or
-// failure based on whether the TEST_PILOT environment variable is
-// set.  The default is to report the test as successful.  Enabling
-// FOLLY_SKIP_AS_FAILURE can be useful with a test harness that can
-// identify the "Test skipped by client" in the failure message and
-// convert this into a skipped test result.
+// It used to be that `googletest` did NOT have a built-in mechanism to
+// report tests as skipped a run time.  As of the following commit, it has
+// fairly complete support for the feature -- i.e.  `SKIP` does what you
+// expect both in test fixture `SetUp` and in `Environment::SetUp`, as well
+// as in the test body:
+//
+// https://github.com/google/googletest/commit/67c75ff8baf4228e857c09d3aaacd3f1ddf53a8f
+//
+// Open-source projects depending on folly may still use the latest release
+// googletest-1.8.1, which does not have that feature.  Therefore, for
+// backwards compatibility, our `SKIP` only uses `GTEST_SKIP_` when
+// available.
+//
+// Difference from vanilla `GTEST_SKIP`: The skip message diverges from
+// upstream's "Skipped" in order to conform with the historical message in
+// `folly`.  The intention is not to break tooling that depends on that
+// specific pattern.
+//
+// Differences between the new `GTEST_SKIP_` path and the old
+// `GTEST_MESSAGE_` path:
+//   - New path: `SKIP` in `SetUp` prevents the test from running.  Running
+//     the gtest main directly clearly marks SKIPPED tests.
+//   - Old path: Without `skipIsFailure`, `SKIP` in `SetUp` would
+//     unexpectedly execute the test, potentially causing segfaults or
+//     undefined behavior.  We would either report the test as successful or
+//     failed based on whether the `FOLLY_SKIP_AS_FAILURE` environment
+//     variable is set.  The default is to report the test as successful.
+//     Enabling FOLLY_SKIP_AS_FAILURE was to be used with a test harness
+//     that can identify the "Test skipped by client" in the failure message
+//     and convert this into a skipped test result.
+#ifdef GTEST_SKIP_
+#define SKIP(msg) GTEST_SKIP_("Test skipped by client")
+#else
 #define SKIP()                                       \
   GTEST_AMBIGUOUS_ELSE_BLOCKER_                      \
   return GTEST_MESSAGE_(                             \
@@ -59,6 +85,7 @@
       ::folly::test::detail::skipIsFailure()         \
           ? ::testing::TestPartResult::kFatalFailure \
           : ::testing::TestPartResult::kSuccess)
+#endif
 
 // Encapsulate conditional-skip, since it's nontrivial to get right.
 #define SKIP_IF(expr)           \