Finalize goedesy tests with Google Test #278
Conversation
mwoehlke-kitware
force-pushed
the
dev/geo_conv-complete-tests-with-gtest
branch
from
5a25caa
to
cedcb99
Compare
|
It appears that the Ubuntu gtest package is not actually usable; therefore, this is blocked on Kitware/fletch#156. |
mwoehlke-kitware
force-pushed
the
dev/geo_conv-complete-tests-with-gtest
branch
2 times, most recently
from
c3053a6
to
c36f966
Compare
| TEST_ERROR("The default point is not empty"); | ||
| } | ||
| // Paranoia-check initial state | ||
| EXPECT_TRUE( p.is_empty() ); |
There was a problem hiding this comment.
I assume the GTest output provides enough context (file and line number) in the output to distinguish which test is failing. For example, line 96 below has the identical test, but in a different context. In converting to GTest you are discarding the error message strings that provided more context to error. Are we okay with that? This is more of a group opinion poll than a particular suggestion. I guess if I can track a failure back to a particular line in file that is sufficient. But I might immediate know what is wrong if I get more context in the failure message.
There was a problem hiding this comment.
Yes; that's one of the good things about Google Test; assertions contain the location of the failure, and generally both the text arguments as well as evaluated values of the predicate. For example, after monkeying with the preceding code so that this assertion fails, I get:
path/to/kwiver/vital/tests/test_geo_point.cxx:84: Failure
Value of: p.is_empty()
Actual: false
Expected: true
My philosophy is generally to assume that if an assertion fails, you are at least going to look at that assertion in the source code, and that redundantly providing information that is readily apparent from that context is, well, redundant 😄. Mostly because this significantly cuts down on the time it takes to write unit tests. That's not to say I'd be strongly opposed to other philosophies, however.
(Code-wise, it is trivial to add a more detailed explanation: EXPECT_<PRED>(...) << "details". However, what do I write for "details"? "Default constructed point should be empty"? "Default constructed point is empty"? "Default constructed point is not empty"? Do I say "point", or "geo_point"? "vital::geo_point"? Do I capitalize it? End with a period? Already with TEST_ERROR we aren't fully consistent in answering such questions. Taking the approach that we only add details when they aren't obvious from looking at the assertion in its source code context allows these questions to be ignored 95% of the time.)
There was a problem hiding this comment.
I guess it doesn't matter much as long as the test code is documented well enough to be clear what the test is trying to test. I can imagine someone writing a long sequence of EXPECT_* statements without documenting what the test is doing. The old framework forced your to write something for each test. That said, commentary is not always needed for every test call. A comment covering a block of tests, as you've done here, is probably sufficient.
|
How about a release note about the change to Google Test? Otherwise looks good to me |
Change geo_polygon test to use Google Test, and finish implementing its test cases. For now, the machinery to create the test executable is very crude; this will be cleaned up in a subsequent commit.
Import GoogleTest module from CMake 3.10. This provides dynamic discovery of tests using Google Test, rather than the static discovery used by older versions. Dynamic discovery is more robust and can handle corner cases (particularly with respect to parameterized tests) that static discovery cannot. Note that this is not an exact copy of the upstream version, as upstream uses CTEST_INCLUDE_FILES, which is also new in CMake 3.10. This version employs a work-around using the older CTEST_INCLUDE_FILE. This has some limitations if CTEST_INCLUDE_FILE is being used otherwise, but KWIVER is not doing so and should thus be unaffected. See also https://gitlab.kitware.com/cmake/cmake/merge_requests/1056.
Add a small helper script to identify "future" CMake modules and automatically import them as needed.
Update helpers for adding tests using Google Test to use the new discovery mechanism.
Add some unit tests of the new UTM/UPS zone determination function introduced in the previous commit.
Import FindGTest.cmake from CMake 3.9 into our future modules framework. This ensures that FindGTest.cmake will provide imported targets, which our future-imported test discovery relies upon, and also that we have a version of the find module from which the helper functions have been split off into a separate module.
Update release notes to mention new dependency on Google Test and that we are transitioning to it as our primary testing framework.
mwoehlke-kitware
force-pushed
the
dev/geo_conv-complete-tests-with-gtest
branch
from
232b1f7
to
3f4d9c8
Compare
Done. Remind me to change the "in the process" wording before closing #277 😉. |
See #277 for major background.
This is the first in a series of PR's to port kwiver's unit tests to Google Test. This PR implements missing bits of the
geo_polygontest (which was the impetus for using Google Test in the first place), ports thegeo_pointtest, and adds the CMake infrastructure for using Google Test.As noted in #277, the porting is being split into multiple PR's in order to make review easier.