GTEST_SKIP doesn't work in test environments #2189

asomers · 2019-03-20T19:56:21Z

GTEST_SKIP() works great when used in fixtures. But it doesn't seem to have any effect when used in a test environment. I would expect that every test case in the program gets skipped if an environment calls GTEST_SKIP().

Example program:

#include "gtest/gtest.h"

using ::testing::Test;

class SkipEnvironment : public testing::Environment {
 public:
  void SetUp() override {
    GTEST_SKIP() << "Skipping the entire environment";
  }
};

TEST(SkipTest, ShouldBeSkippedByEnvironment) {
  EXPECT_EQ(0, 1);
}

int main(int argc, char **argv) {
  testing::InitGoogleTest(&argc, argv);

  SkipEnvironment* const env = new SkipEnvironment;
  testing::AddGlobalTestEnvironment(env);

  return (RUN_ALL_TESTS());

Actual output:

Executing tests from //googletest/test:gtest_skip_environment_test
-----------------------------------------------------------------------------
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from SkipTest
[ RUN      ] SkipTest.ShouldBeSkippedByEnvironment
googletest/test/gtest_skip_environment_test.cc:16: Failure
Expected equality of these values:
  0
  1
[  FAILED  ] SkipTest.ShouldBeSkippedByEnvironment (0 ms)
[----------] 1 test from SkipTest (0 ms total)
  
[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (0 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SkipTest.ShouldBeSkippedByEnvironment

 1 FAILED TEST

cc @ngie-eign

The text was updated successfully, but these errors were encountered:

ngie-eign · 2019-03-30T01:41:25Z

This seems like it's parallel to the issue I reported in #2169.

ngie-eign · 2019-03-30T02:04:49Z

Ok, interesting note. If we used GTEST_FAIL, it would bail out here. Let's give this a shot...

$ ./setup_environment_nonfatal_fail_demo 
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
setup_environment_nonfatal_fail_demo.cc:6: Failure
Expected equality of these values:
  false
  true
[----------] 2 tests from Test
[ RUN      ] Test.AlwaysPasses
[       OK ] Test.AlwaysPasses (0 ms)
[ RUN      ] Test.AlwaysFails
setup_environment_nonfatal_fail_demo.cc:15: Failure
Expected equality of these values:
  true
  false
[  FAILED  ] Test.AlwaysFails (0 ms)
[----------] 2 tests from Test (0 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (0 ms total)
[  PASSED  ] 1 test.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] Test.AlwaysFails

 1 FAILED TEST
$ ./setup_environment_fatal_fail_demo 
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
setup_environment_fatal_fail_demo.cc:6: Failure
Failed
Calling a fatal failure
[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (0 ms total)
[  PASSED  ] 2 tests.
[  FAILED  ] 0 tests, listed below:

 0 FAILED TESTS

Oh wow, that is confusing, but it seems to work!

This aligns with what's currently documented in googletest/docs/advanced.md:

1291 Now, when `RUN_ALL_TESTS()` is called, it first calls the `SetUp()` method of
1292 the environment object, then runs the tests if there was no fatal failures, and
1293 finally calls `TearDown()` of the environment object.

I know where I need to add the check. PR coming up soon.

@asomers

These were used in illustrating the issue discussed in: google/googletest#2189 . While here, remove a duplicate "#define" for `GTEST_THREADSAFE` and move setup_environment_fail_demo.cc to setup_environment_nonfatal_fail_demo.cc, since I now know that I was testing for non-fatal failures (which don't cause the test to abort). Contributed (in part) by @asomers.

ngie-eign · 2019-03-30T03:09:45Z

Good news is, now it stops when GTEST_SKIP is triggered! Bad news is, the results are confusing as hell :P!

$ ./googlemock/gtest/gtest_skip_in_environment_setup_test 
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
Should omit a skip message below
[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 2 tests.

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. Thus, making `Environment::SetUp()` handle `GTEST_SKIP()`, by not executing the testcases, is desirable. 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 google#2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

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 google#2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

ngie-eign · 2019-03-30T18:33:56Z

Good news is, now it stops when GTEST_SKIP is triggered! Bad news is, the results are confusing as hell :P!

...

I fixed the fact that it wasn't dumping out diagnostic information in #2203.

@EricWF, @gennadiycivil: please review the change.

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 google#2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

…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. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765 git-svn-id: svn+ssh://svn.freebsd.org/base/head@345770 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…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. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765

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 google#2189. Signed-off-by: Enji Cooper <yaneurabeya@gmail.com>

ngie-eign · 2019-04-04T18:29:54Z

@asomers: this can be closed.

…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. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765 git-svn-id: https://svn.freebsd.org/base/head@345770 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…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. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765 git-svn-id: svn+ssh://svn.freebsd.org/base/head@345770 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f

…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. google/googletest#2203 Reported by: asomers (google/googletest#2189) Reviewed by: asomers Approved by: emaste (mentor) MFC after: 2 months Differential Revision: https://reviews.freebsd.org/D19765

forthright48 · 2019-07-03T14:35:00Z

@asomers and @ngie-eign : Why was this bug closed? It hasn't been merged to master yet right? I am still facing the same issue: GTEST_SKIP not working in test environments.

asomers · 2019-07-03T15:24:09Z

Yes it has been merged. See PR #2203 .

These tests were disabled because they were flaky on Win7 with TIME_OUT. However, these tests should get skipped. I can't repro this on Win 7 anymore, even sync to the same revision. (https://chromium-swarm.appspot.com/task?id=48a8456871114310) This is an attempt to fix the issue similar to https://cs.chromium.org/chromium/src/chrome/browser/vr/test/xr_browser_test.cc?l=166&rcl=5e8d5843886822da7d70bd23db473ab5ea47e3a2 Call GTEST_SKIP has been an issue for googletest, see google/googletest#2189 And I verified skip can work on Win7 with unittest. TEST: https://chromium-swarm.appspot.com/task?id=48a3f0a5e08c7610 Bug: 1025335 Change-Id: I906a799810d61952198ca1a57cf73e2414ca2611 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1925135 Reviewed-by: Mihai Sardarescu <msarda@chromium.org> Commit-Queue: Sven Zheng <svenzheng@chromium.org> Auto-Submit: Sven Zheng <svenzheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#718217}

ngie-eign mentioned this issue Mar 30, 2019

Handle GTEST_SKIP() when calling Environment::SetUp() #2203

Merged

asomers closed this as completed Apr 4, 2019

ocroquette mentioned this issue Jun 25, 2019

googletest should release a new version #2267

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GTEST_SKIP doesn't work in test environments #2189

GTEST_SKIP doesn't work in test environments #2189

asomers commented Mar 20, 2019

ngie-eign commented Mar 30, 2019 •

edited

ngie-eign commented Mar 30, 2019 •

edited

ngie-eign commented Mar 30, 2019

ngie-eign commented Mar 30, 2019

ngie-eign commented Apr 4, 2019

forthright48 commented Jul 3, 2019

asomers commented Jul 3, 2019

GTEST_SKIP doesn't work in test environments #2189

GTEST_SKIP doesn't work in test environments #2189

Comments

asomers commented Mar 20, 2019

ngie-eign commented Mar 30, 2019 • edited

ngie-eign commented Mar 30, 2019 • edited

ngie-eign commented Mar 30, 2019

ngie-eign commented Mar 30, 2019

ngie-eign commented Apr 4, 2019

forthright48 commented Jul 3, 2019

asomers commented Jul 3, 2019

ngie-eign commented Mar 30, 2019 •

edited

ngie-eign commented Mar 30, 2019 •

edited