-
Notifications
You must be signed in to change notification settings - Fork 697
Replace CppUnit with GoogleTest in AX tests #1919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Replace CppUnit with GoogleTest in AX tests #1919
Conversation
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. |
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this approach, it means downstream software now needs to also install and include gtest to use AX due to the header requirements. I would keep the old friend system and introduce a separate class which TestLogger can use to access the private members, something like:
struct TestAccessor;
struct MyClass
{
private:
friend class ::TestAccessor;
float private_member;
};
struct TestAccessor
{
TestAccessor(MyClass& in) : a(in) {}
auto get_member() { return a.private_member; }
MyClass& a;
};
void Test()
{
MyClass a;
TestAccessor acc(a);
acc.get_member();
}
Alternative we have to muck around with macros to ifdef out gtest in prod (non-test) builds - I think I prefer the prior.
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. |
…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]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
Signed-off-by: Tim Straubinger <[email protected]>
1de8690
to
692d63e
Compare
Changes have been rebased onto the current master branch and I've included removing CppUnit as a dependency. Whitespace that was causing a linting check to fail has also been removed. Unfortunately I don't know where to see other failed checks now that newer changes have been pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tstraubinger thank you very much for these changes - on the whole these look great, there's a few files I need to look at a bit closer (mainly TestFunctionTypes due to the logic switch within the non-void lambdas), but my only comment is to avoid including gtest in the core ax lib Logger/Executables files (re-work the friend setup)
friend class ::TestLogger; | ||
FRIEND_TEST(TestLogger, testParseSetsTree); | ||
FRIEND_TEST(TestLogger, testAddError); | ||
FRIEND_TEST(TestLogger, testAddWarning); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of this approach, it means downstream software now needs to also install and include gtest to use AX due to the header requirements. I would keep the old friend system and introduce a separate class which TestLogger can use to access the private members, something like:
struct TestAccessor;
struct MyClass
{
private:
friend class ::TestAccessor;
float private_member;
};
struct TestAccessor
{
TestAccessor(MyClass& in) : a(in) {}
auto get_member() { return a.private_member; }
MyClass& a;
};
void Test()
{
MyClass a;
TestAccessor acc(a);
acc.get_member();
}
Alternative we have to muck around with macros to ifdef out gtest in prod (non-test) builds - I think I prefer the prior.
friend class ::TestPointExecutable; | ||
|
||
FRIEND_TEST(TestPointExecutable, testConstructionDestruction); | ||
FRIEND_TEST(TestPointExecutable, testAttributeCodecs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about friend impl
@@ -293,7 +295,8 @@ class OPENVDB_AX_API VolumeExecutable | |||
|
|||
private: | |||
friend class Compiler; | |||
friend class ::TestVolumeExecutable; | |||
|
|||
FRIEND_TEST(TestVolumeExecutable, testConstructionDestruction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about friend impl
} else if (arg == "-v") { | ||
verbose = true; | ||
} else if (arg == "-g") { | ||
sGenerateAX = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be callable from the test binary - in practice we don't use it very much, so in the rare cases where the tests need to be re-generated, I think it's fine for this to need a re-compile. So just commenting for future visibility, nothing else to do
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