Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Upgrade client active object mgr tests to Catch2 #13609

Closed
wants to merge 4 commits into from
Closed

Upgrade client active object mgr tests to Catch2 #13609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a19ba0e
Upgrade client active object mgr tests to Catch2
JosiahWI Jun 19, 2023
bc1db89
Increment failed modules count for Catch2 tests
JosiahWI Oct 9, 2023
4abcb99
Fix typo
JosiahWI Oct 9, 2023
47e88c6
Fix rebase mistakes
JosiahWI Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ if(NOT USE_LUAJIT)
add_subdirectory(lib/bitop)
endif()

if(BUILD_BENCHMARKS)
if(BUILD_UNITTESTS OR BUILD_BENCHMARKS)
add_subdirectory(lib/catch2)
endif()

Expand Down
3 changes: 2 additions & 1 deletion lib/catch2/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@
# + return os << std::fixed << duration.value() << ' ' << duration.unitsAsString();

add_library(catch2 INTERFACE)
target_include_directories(catch2 INTERFACE ${CMAKE_CURRENT_SOURCE_DIR})
add_library(Catch2::Catch2 ALIAS catch2)
target_include_directories(catch2 INTERFACE "${CMAKE_CURRENT_SOURCE_DIR}")
8 changes: 4 additions & 4 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -593,8 +593,8 @@ if(BUILD_CLIENT)
if (USE_SPATIAL)
target_link_libraries(${PROJECT_NAME} ${SPATIAL_LIBRARY})
endif()
if(BUILD_BENCHMARKS)
target_link_libraries(${PROJECT_NAME} catch2)
if(BUILD_UNITTESTS OR BUILD_BENCHMARKS)
target_link_libraries(${PROJECT_NAME} Catch2::Catch2)
endif()
endif(BUILD_CLIENT)

Expand Down Expand Up @@ -655,8 +655,8 @@ if(BUILD_SERVER)
${CURL_LIBRARY}
)
endif()
if(BUILD_BENCHMARKS)
target_link_libraries(${PROJECT_NAME}server catch2)
if(BUILD_UNITTESTS OR BUILD_BENCHMARKS)
target_link_libraries(${PROJECT_NAME}server Catch2::Catch2)
endif()
endif(BUILD_SERVER)

Expand Down
1 change: 0 additions & 1 deletion src/activeobjectmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class TestServerActiveObjectMgr;
template <typename T>
class ActiveObjectMgr
{
friend class ::TestClientActiveObjectMgr;
friend class ::TestServerActiveObjectMgr;

public:
Expand Down
10 changes: 9 additions & 1 deletion src/unittest/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@ with this program; if not, write to the Free Software Foundation, Inc.,
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/

#define CATCH_CONFIG_RUNNER
#include <catch.hpp>

#include "test.h"

#include "client/sound.h"
Expand Down Expand Up @@ -249,7 +252,12 @@ bool run_tests()
<< num_total_tests_run << " failed individual tests)." << std::endl
<< " Testing took " << tdiff << "ms total." << std::endl
<< "++++++++++++++++++++++++++++++++++++++++"
<< "++++++++++++++++++++++++++++++++++++++++" << std::endl;
<< "++++++++++++++++++++++++++++++++++++++++" << std::endl
<< "Catch test results: " << std::endl;
auto catch_test_failures = Catch::Session().run();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this just runs the other tests unconditionally after the normal ones with no integration with --test-module

if (catch_test_failures > 0) {
++num_modules_failed;
}

return num_modules_failed == 0;
}
Expand Down
119 changes: 55 additions & 64 deletions src/unittest/test_clientactiveobjectmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ with this program; if not, write to the Free Software Foundation, Inc.,
51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
*/

#include "client/activeobjectmgr.h"
#include <algorithm>
#include "test.h"

#include "profiler.h"
#include "client/activeobjectmgr.h"

#include <catch.hpp>

#include <utility>

class TestClientActiveObject : public ClientActiveObject
{
Expand Down Expand Up @@ -58,85 +60,74 @@ class TestClientActiveObjectMgr : public TestBase

void runTests(IGameDef *gamedef);

void testFreeID();
void testRegisterObject();
void testRemoveObject();
void testGetActiveSelectableObjects();
};

static TestClientActiveObjectMgr g_test_instance;

void TestClientActiveObjectMgr::runTests(IGameDef *gamedef)
{
TEST(testFreeID);
TEST(testRegisterObject)
TEST(testRemoveObject)
TEST(testGetActiveSelectableObjects)
}

////////////////////////////////////////////////////////////////////////////////

void TestClientActiveObjectMgr::testFreeID()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the plan if any converted tests need to access the gamedef?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. The plan is to move the TestGameDef to its own file and instantiate an instance of it for each Catch test that needs it. If that proves to be too slow, then I think mocking some parts of the gamedef might be appropriate. That's more work than a global gamedef, which almost looks like an amazing solution, but there's a big danger with that because the global instance might be modified by a test and not completely reset at the end. The tests would no longer be independent of each other and the order in which the tests are run could start to matter, which is a big mess. Looking at the code, it seems we could already have this problem.

{
client::ActiveObjectMgr caomgr;
std::vector<u16> aoids;

u16 aoid = caomgr.getFreeId();
// Ensure it's not the same id
UASSERT(caomgr.getFreeId() != aoid);

aoids.push_back(aoid);

// Register basic objects, ensure we never found
for (u8 i = 0; i < UINT8_MAX; i++) {
// Register an object
auto tcao_u = std::make_unique<TestClientActiveObject>();
auto tcao = tcao_u.get();
caomgr.registerObject(std::move(tcao_u));
aoids.push_back(tcao->getId());

// Ensure next id is not in registered list
UASSERT(std::find(aoids.begin(), aoids.end(), caomgr.getFreeId()) ==
aoids.end());
}

caomgr.clear();
static TestClientActiveObject* register_default_test_object(
client::ActiveObjectMgr &caomgr) {
auto test_obj = std::make_unique<TestClientActiveObject>();
auto result = test_obj.get();
REQUIRE(caomgr.registerObject(std::move(test_obj)));
return result;
}

void TestClientActiveObjectMgr::testRegisterObject()
TEST_CASE("test client active object manager")
{
client::ActiveObjectMgr caomgr;
auto tcao_u = std::make_unique<TestClientActiveObject>();
auto tcao = tcao_u.get();
UASSERT(caomgr.registerObject(std::move(tcao_u)));

u16 id = tcao->getId();

auto tcaoToCompare = caomgr.getActiveObject(id);
UASSERT(tcaoToCompare->getId() == id);
UASSERT(tcaoToCompare == tcao);

tcao_u = std::make_unique<TestClientActiveObject>();
tcao = tcao_u.get();
UASSERT(caomgr.registerObject(std::move(tcao_u)));
UASSERT(caomgr.getActiveObject(tcao->getId()) == tcao);
UASSERT(caomgr.getActiveObject(tcao->getId()) != tcaoToCompare);

caomgr.clear();
}

void TestClientActiveObjectMgr::testRemoveObject()
{
client::ActiveObjectMgr caomgr;
auto tcao_u = std::make_unique<TestClientActiveObject>();
auto tcao = tcao_u.get();
UASSERT(caomgr.registerObject(std::move(tcao_u)));
auto tcao1 = register_default_test_object(caomgr);

SECTION("When we register a client object, "
"then it should be assigned a unique ID.")
{
for (int i = 0; i < UINT8_MAX; ++i) {
auto other_tcao = register_default_test_object(caomgr);
CHECK(other_tcao->getId() != tcao1->getId());
}
}

u16 id = tcao->getId();
UASSERT(caomgr.getActiveObject(id) != nullptr)
SECTION("two registered objects")
{
auto tcao2 = register_default_test_object(caomgr);
auto tcao2_id = tcao2->getId();

auto obj1 = caomgr.getActiveObject(tcao1->getId());
REQUIRE(obj1 != nullptr);

auto obj2 = caomgr.getActiveObject(tcao2_id);
REQUIRE(obj2 != nullptr);

SECTION("When we query an object by its ID, "
"then we should get back an object with that ID.")
{
CHECK(obj1->getId() == tcao1->getId());
CHECK(obj2->getId() == tcao2->getId());
}

SECTION("When we register and query for an object, "
"its memory location should not have changed.")
{
CHECK(obj1 == tcao1);
CHECK(obj2 == tcao2);
}
}

caomgr.removeObject(tcao->getId());
UASSERT(caomgr.getActiveObject(id) == nullptr)
SECTION("Given an object has been removed, "
"when we query for it, "
"then we should get nullptr.")
{
auto id = tcao1->getId();
caomgr.removeObject(tcao1->getId()); // may invalidate tcao1
CHECK(caomgr.getActiveObject(id) == nullptr);
}

caomgr.clear();
}
Expand Down