Replace CppUnit with GoogleTest in AX tests #1919
Conversation
…l conversion; convert TestPrinters to gtest Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
…tions to Logger Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
…d declarations to PointExecutable Signed-off-by: Tim Straubinger <[email protected]>
…nd declarations to VolumeExecutable Signed-off-by: Tim Straubinger <[email protected]>
… gtest and cppunit to test/util.h Signed-off-by: Tim Straubinger <[email protected]>
…cppunit dual support from utils.h Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
tstraubinger
changed the title
Signed-off-by: Tim Straubinger <[email protected]>
|
These changes are ready for review. I've completely removed CppUnit from the AX tests but it remains a dependency at the top level. I think I'll put in a second pull request to remove it there as well, separately from this one. |
| @@ -173,7 +48,7 @@ inline void testFunctionOptions(unittest_util::AXTestHarness& harness, | |||
| const ast::Tree::Ptr syntaxTree = ast::parse(code.c_str()); | |||
| timer.stop(); | |||
|
|
|||
| CPPUNIT_ASSERT_MESSAGE(syntaxTree, "Invalid AX passed to testFunctionOptions."); | |||
| ASSERT_TRUE(syntaxTree) << "Invalid AX passed to testFunctionOptions."; | |||
There was a problem hiding this comment.
The arguments to CPPUNIT_ASSERT_MESSAGE were reversed here. If I understand correctly then, this was ensuring that the string literal evaluates to true (which as a non-null pointer, it does) and would otherwise have printed out the pointer to the AST (perhaps via shared_ptr's operator<<(ostream&))
| // NOTE: GTest's ASSERT_* macros create a 'return;' statement | ||
| // which errors here. Exceptions are being used instead to | ||
| // exit early. | ||
|
|
||
| // test the builder is pointing to the correct location | ||
| CPPUNIT_ASSERT(&B != &_B); | ||
| if (&B == &_B) | ||
| { | ||
| throw std::runtime_error("Builder should be different"); | ||
| } |
There was a problem hiding this comment.
Some opinionated changes: GTest ASSERT_* macros can't be used in a non-void function because they expand to a void return statement (I suppose Google wanted to support code bases with C++ exceptions disabled). I've replaced the assertions with equivalent if-statements and explicit C++ exceptions. I've confirmed that GTest reports a failure as expected when these are thrown here.
| friend class ::TestLogger; | ||
| FRIEND_TEST(TestLogger, testParseSetsTree); | ||
| FRIEND_TEST(TestLogger, testAddError); | ||
| FRIEND_TEST(TestLogger, testAddWarning); |
There was a problem hiding this comment.
It might have been possible to avoid these changes to a header file in order to test private members, but this is the GTest-recommended approach. See https://google.github.io/googletest/advanced.html#testing-private-code
|
I've created a secondary branch off of this one that further removes all mentions of CppUnit from the build dependencies and instructions. Shall I create a merge request into this branch, wait until this is merged, or just push the changes to this branch? |
Thanks, I'm fine to have these changes in this branch as they're all closely related. I've just approved the CI running, but note there are some conflicts, so this branch likely needs to be updated and the conflicts resolved which may affect the result of the CI. |
This is a work-in-progress pull request. I've begun by adding GoogleTest to the AX unit test runner and am currently running both the existing CppUnit test suite as well as any added GoogleTest tests. The intention is to migrate the test cases one-by-one and then remove CppUnit completely afterwards.
Resolves #1917