deepmodeling / abacus-develop

An electronic structure package based on either plane wave basis or numerical atomic orbitals.
http://abacus.ustc.edu.cn
GNU Lesser General Public License v3.0
173 stars 134 forks source link

Method of visiting private members in unit test #2666

Closed kirk0830 closed 1 year ago

kirk0830 commented 1 year ago

Meet trouble when adding unit test on module_io/bessel_basis. Relevant issue #2614 .

Discussed in https://github.com/deepmodeling/abacus-develop/discussions/2659

Originally posted by **kirk0830** June 21, 2023 The writing and debugging unit test of module_io/bessel_basis are still in progress, while thanks to Zuxin we will have a brand new module (module_nao) later, therefore the unit test of bessel_basis may be not quite important if the new module is fully implemented. However, presently, during adding unit tests on private members, the use of `#define private public` will iteratively cause errors like: `error: ‘struct std::__cxx11::basic_stringbuf<_CharT, _Traits, _Alloc>::__xfer_bufptr ’ redeclared with different access` ` 446 | struct __xfer_bufptrs` ` | ^~~~~~` which is, obviously caused by imposing `#define private public` onto `` (for analysis with more details posted by other coder, see [why cannot use #define private public anymore?](https://www.cnblogs.com/sdu20112013/p/11803295.html)). Got advice from Tianqi that removing unnecessary dependence of header files on other header files may finally solve this error, but this error pops again and again, from `global_function.h` to `global_variable.h`, then `realarray.h`, `unitcell.h`, `vector3.h` and `matrix3.h`. Actually there are also other methods for visiting private variables and functions, but will be more or less invasive, such as gtest/gtest_prod.h::FRIEND_TEST, or the following way: 1. first create a variable when setting BUILD_TESTING option in CMakeLists.txt is true, say __UNITTEST 2. In header file, use #ifdef __UNITTEST #define PRIVATE_SWTICH public #else #define PRIVATE_SWTICH private #endif and substitute private with PRIVATE_SWTICH. The use of FRIEND_TEST also requires declaration in private scope of class to test. So these two methods are more or less invasive but has pros that makes the range of variables which will be set as public during unit test controllable. Suggestions provided by ChatGPT4 are pasted here: >It looks like the error is caused due to an access level conflict for `struct __xfer_bufptrs` within the `basic_stringbuf` class, which is part of the `std` namespace (typically declared in the `` header file). The most likely reason for this error is the `#define private public` directive. >While this directive can help you access private members of a class for testing purposes, it also changes the access level of everything in every file that is included after the directive, not just your own classes. This can cause conflicts with library classes, such as `std::basic_stringbuf`, that rely on the proper access levels for their members. >This situation is a perfect example of the risks associated with using `#define private public`. As I suggested earlier, safer alternatives for testing private methods are refactoring your code to make it more testable, making the test a friend of the class, or using the Google Test `FRIEND_TEST` macro. >As a final note, the use of `#define private public` is generally considered a bad practice because it breaks the encapsulation principle of object-oriented programming. Moreover, it can lead to issues like the one you're experiencing, especially in complex projects that involve multiple libraries or large codebases. >It's always better to architect your classes and methods in such a way that you can test them without needing to access private members. This typically involves designing your classes with testability in mind, for example by providing public methods that can be used to check the state of the object, or by using interfaces and dependency injection so you can provide mock implementations for testing. Therefore comments and opinions are of need and will be appreciated about method visiting private variables and functions during unit test.
jinzx10 commented 1 year ago

After some search, I find that access check is not invoked in explicit template instantiation, and there's already some repo/code available on github which exploit this behavior for non-invasive tests:

https://github.com/martong/access_private https://gist.github.com/dabrahams/1528856

This behavior is stated in the following paragraph from the C++11 standdard (14.7.2, paragraph 12) https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf

"The usual access checking rules do not apply to names used to specify explicit instantiations. [ Note: In particular, the template arguments and names used in the function declarator (including parameter types, return types and exception specifications) may be private types or objects which would normally not be accessible and the template may be a member template or member function which would not normally be accessible. — end note ]"

jinzx10 commented 1 year ago

The tool in https://github.com/martong/access_private is simply one header file, and the usage is non-invasive, i.e., no modification to the origional source file is required. I think if testing private functions is really necessary, this might be something we can introduce (or simply learn the idea & write our own) to our unit test procedure. What are your opinions @baixiaokuang @hongriTianqi ?

The following is an example with googletest.

PS: The test/test.cpp in https://github.com/martong/access_private contains more examples, including static member & functions, etc.

testee.h (the class under test)

#ifndef TESTEE_H_
#define TESTEE_H_

#include <iostream>

class Testee {
public:
    Testee() : num_(0) {}
    ~Testee() {};

    void set(int i) { num_ = i; }
    int get() const { return num_; }

private:
    int num_;

    void addone() { num_ += 1; }
    int times_a_plus_b(int a, int b) { return a * num_ + b; }
};
#endif

unit test source file:

#include "testee.h"
#include "../include/access_private.hpp"

#include <gtest/gtest.h>

ACCESS_PRIVATE_FUN(Testee, void(), addone);
ACCESS_PRIVATE_FUN(Testee, int(int,int), times_a_plus_b);
ACCESS_PRIVATE_FIELD(Testee, int, num_)

class Tester : public ::testing::Test {

protected:
    void SetUp();
    void TearDown();

    Testee t;
};

void Tester::SetUp() {
    t.set(0);
}

void Tester::TearDown() {
}

TEST_F(Tester, test1) {
    int n = 9;

    t.set(n);
    call_private::addone(t);
    EXPECT_EQ(t.get(), n+1);

    t.set(n);
    int a = 2;
    int b = 5;
    int res = call_private::times_a_plus_b(t, a, b);
    EXPECT_EQ(res, a*n+b);

    n = 22;
    t.set(n);
    EXPECT_EQ(access_private::num_(t), n);
}

int main(int argc, char **argv) {
    testing::InitGoogleTest(&argc, argv);
    int result = RUN_ALL_TESTS();
    return result;
}
hongriTianqi commented 1 year ago

@jinzx10 @kirk0830 Good Idea. I think we should learn from the author, and implement our own.

kirk0830 commented 1 year ago

Private functions have been tested indirectly by testing public functions which call those private: They are: readin_C4(),init_TableOne(), init_Faln(). Function allocate_C4() should be tested in unit test of ModuleBase::RealArray.