Page MenuHomeFreeBSD

Import proof-of-concept for handling `GTEST_SKIP()` in `Environment::SetUp`
ClosedPublic

Authored by ngie on Mar 30 2019, 7:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 13 2024, 7:46 PM
Unknown Object (File)
Mar 13 2024, 7:46 PM
Unknown Object (File)
Mar 13 2024, 7:46 PM
Unknown Object (File)
Mar 13 2024, 7:46 PM
Unknown Object (File)
Mar 10 2024, 1:46 PM
Unknown Object (File)
Jan 21 2024, 4:53 AM
Unknown Object (File)
Jan 3 2024, 12:40 PM
Unknown Object (File)
Jan 3 2024, 12:40 PM

Details

Summary

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).

  1. https://github.com/google/googletest/pull/2203
Test Plan
$ /usr/tests/lib/googletest/gtest_main/gtest_skip_in_environment_setup_test 
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
Skipped
Skipping the entire environment
[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (0 ms total)
[  PASSED  ] 2 tests.
$ kyua test -k /usr/tests/lib/googletest/gtest_main/Kyuafile gtest_skip_in_environment_setup_test:main
gtest_skip_in_environment_setup_test:main  ->  passed  [0.003s]

Results file id is usr_tests_lib_googletest_gtest_main.20190331-173409-782067
Results saved to /home/ngie/.kyua/store/results.usr_tests_lib_googletest_gtest_main.20190331-173409-782067.db

1/1 passed (0 failed)

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

While this change LGTM and works on my machine, you might want to give upstream a least a few hours to review the PR before you commit it to FreeBSD>

This revision is now accepted and ready to land.Mar 30 2019, 8:11 PM

While this change LGTM and works on my machine, you might want to give upstream a least a few hours to review the PR before you commit it to FreeBSD>

The upstream folks have been lagging on reviews. I have 4 open PRs (some over a month ago), 3 of which can be pulled.

That said, I'll wait for the weekend to pass before pulling in this change.

Annoying. This doesn't work with our franken version of googletest >:(...

$ /usr/tests/lib/googletest/gtest_main/gtest_skip_in_environment_setup_test
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from Test
[ RUN      ] Test.AlwaysPasses
[       OK ] Test.AlwaysPasses (0 ms)
[ RUN      ] Test.AlwaysFails
/usr/src/contrib/googletest/googletest/test/gtest_skip_in_environment_setup_test.cc:51: Failure
Expected equality of these values:
  true
  false
[  FAILED  ] Test.AlwaysFails (1 ms)
[----------] 2 tests from Test (1 ms total)

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

 1 FAILED TEST
$ (cd ~/nfs/git/googletest/; find . -name \*skip\*test)
./googlemock/gtest/gtest_skip_in_environment_setup_test
./googlemock/gtest/gtest_skip_test
[ngie@pinklady-fbsd-current /usr/src]$ (cd ~/nfs/git/googletest/; ./googlemock/gtest/gtest_skip_in_environment_setup_test)
[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
Skipped
Skipping the entire environment
[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (0 ms total)
[  PASSED  ] 2 tests.

Or... somehow the change was missing from the branch I was working on (WTF?!)

Make the backport actually work by including the missing gtest.cc change

This revision now requires review to proceed.Mar 31 2019, 5:32 PM

@asomers: I was missing the gtest.cc hunk -- so obviously this wouldn't have worked if I have committed this to ^/head.

I've tested the complete version and it works on my branch. Could you please re-review this change?

Ok, now I feel pretty dumb. When I said before that the change works for me, I had forgotten that I had already worked around the bug that this change is supposed to fix! But now it actually does work. If I remove the workaround:

> ./xattr
[==========] Running 18 tests from 4 test cases.
[----------] Global test environment set-up.
Skipped
/dev/fuse does not exist
[----------] Global test environment tear-down
[==========] 18 tests from 4 test cases ran. (0 ms total)
[  PASSED  ] 18 tests.

  YOU HAVE 4 DISABLED TESTS

However, if a test gets skipped during the fixture setup instead of the environment, I don't see the skip message. That's probably separate bug though. For example:

> ./write --gtest_filter=WriteBack.rmw 
Note: Google Test filter = WriteBack.rmw
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WriteBack
[ RUN      ] WriteBack.rmw
[  SKIPPED ] WriteBack.rmw (1 ms)
[----------] 1 test from WriteBack (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 0 tests.
[  SKIPPED ] 1 test, listed below:
[  SKIPPED ] WriteBack.rmw

There should've been a message about vfs.fusefs.data_cache_mode.

This revision is now accepted and ready to land.Mar 31 2019, 5:45 PM

Ok, now I feel pretty dumb. When I said before that the change works for me, I had forgotten that I had already worked around the bug that this change is supposed to fix! But now it actually does work. If I remove the workaround:

> ./xattr
[==========] Running 18 tests from 4 test cases.
[----------] Global test environment set-up.
Skipped
/dev/fuse does not exist
[----------] Global test environment tear-down
[==========] 18 tests from 4 test cases ran. (0 ms total)
[  PASSED  ] 18 tests.

  YOU HAVE 4 DISABLED TESTS

However, if a test gets skipped during the fixture setup instead of the environment, I don't see the skip message. That's probably separate bug though. For example:

> ./write --gtest_filter=WriteBack.rmw 
Note: Google Test filter = WriteBack.rmw
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WriteBack
[ RUN      ] WriteBack.rmw
[  SKIPPED ] WriteBack.rmw (1 ms)
[----------] 1 test from WriteBack (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 0 tests.
[  SKIPPED ] 1 test, listed below:
[  SKIPPED ] WriteBack.rmw

There should've been a message about vfs.fusefs.data_cache_mode.

This output suggests that the feature isn't working on the branch.

Do you have the workaround (disabled) checked in to ^/projects/fuse2 ?

In D19765#423836, @ngie wrote:

...

This output suggests that the feature isn't working on the branch.

Do you have the workaround (disabled) checked in to ^/projects/fuse2 ?

Answering myself, here's the code:

https://svnweb.freebsd.org/base/projects/fuse2/tests/sys/fs/fusefs/utils.cc?revision=345626&view=markup&pathrev=345742#l74

Something to note (not sure if it's in the local version you have): virtual void SetUp() { is an abstract method. It should be void SetUp() override {, so it's properly handled as a concrete method.

In D19765#423836, @ngie wrote:

Ok, now I feel pretty dumb. When I said before that the change works for me, I had forgotten that I had already worked around the bug that this change is supposed to fix! But now it actually does work. If I remove the workaround:

> ./xattr
[==========] Running 18 tests from 4 test cases.
[----------] Global test environment set-up.
Skipped
/dev/fuse does not exist
[----------] Global test environment tear-down
[==========] 18 tests from 4 test cases ran. (0 ms total)
[  PASSED  ] 18 tests.

  YOU HAVE 4 DISABLED TESTS

However, if a test gets skipped during the fixture setup instead of the environment, I don't see the skip message. That's probably separate bug though. For example:

> ./write --gtest_filter=WriteBack.rmw 
Note: Google Test filter = WriteBack.rmw
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from WriteBack
[ RUN      ] WriteBack.rmw
[  SKIPPED ] WriteBack.rmw (1 ms)
[----------] 1 test from WriteBack (1 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 0 tests.
[  SKIPPED ] 1 test, listed below:
[  SKIPPED ] WriteBack.rmw

There should've been a message about vfs.fusefs.data_cache_mode.

This output suggests that the feature isn't working on the branch.

Do you have the workaround (disabled) checked in to ^/projects/fuse2 ?

I _do_ see the message when I skip in the TestEnvironment. I just don't see it when I skip in the fixture's SetUp method. Is your change supposed to print skip messages from the SetUp method?

...

I _do_ see the message when I skip in the TestEnvironment. I just don't see it when I skip in the fixture's SetUp method. Is your change supposed to print skip messages from the SetUp method?

It should print from TestEnvironment::SetUp(..) because the tests are being skipped from there, not the per-test suite fixture.

In D19765#423839, @ngie wrote:

...

I _do_ see the message when I skip in the TestEnvironment. I just don't see it when I skip in the fixture's SetUp method. Is your change supposed to print skip messages from the SetUp method?

It should print from TestEnvironment::SetUp(..) because the tests are being skipped from there, not the per-test suite fixture.

Oh, I think I understand your concern now. Let me do some checking.

In D19765#423840, @ngie wrote:

...

Oh, I think I understand your concern now. Let me do some checking.

Ugh. There's another bug in googletest on master; this test should output "skipping all tests from this fixture", but doesn't:

$ /home/ngie/nfs/git/googletest/googlemock/gtest/gtest_skip_test                                                                                                      
Running main() from /home/ngie/nfs/git/googletest/googletest/src/gtest_main.cc
[==========] Running 3 tests from 2 test suites.
[----------] Global test environment set-up.
[----------] 1 test from SkipTest
[ RUN      ] SkipTest.DoesSkip
[  SKIPPED ] SkipTest.DoesSkip (0 ms)
[----------] 1 test from SkipTest (0 ms total)

[----------] 2 tests from Fixture
[ RUN      ] Fixture.SkipsOneTest
[  SKIPPED ] Fixture.SkipsOneTest (0 ms)
[ RUN      ] Fixture.SkipsAnotherTest
[  SKIPPED ] Fixture.SkipsAnotherTest (0 ms)
[----------] 2 tests from Fixture (0 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 2 test suites ran. (0 ms total)
[  PASSED  ] 0 tests.
[  SKIPPED ] 3 tests, listed below:
[  SKIPPED ] SkipTest.DoesSkip
[  SKIPPED ] Fixture.SkipsOneTest
[  SKIPPED ] Fixture.SkipsAnotherTest
In D19765#423841, @ngie wrote:

...

Ugh. There's another bug in googletest on master; this test should output "skipping all tests from this fixture", but doesn't:

Filed upstream as https://github.com/google/googletest/issues/2208 .

Approved.

Please reference the upstream pull request in the commit (perhaps mentioning that upstream has been slow on addressing PRs).

This revision was automatically updated to reflect the committed changes.