From 12983748187c46a37a12e37f72c75b84bf33ca8b Mon Sep 17 00:00:00 2001 From: JosiahWI <41302989+JosiahWI@users.noreply.github.com> Date: Wed, 22 May 2024 11:39:53 -0500 Subject: [PATCH] Upgrade client active object mgr tests to Catch2 (#14565) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Upgrade client active object mgr tests to Catch2 In addition to invoking Catch2's test runner after Minetest's homemade runner, this refactors the tests to follow the DRY principle, and gives them expressive names and clear assertions. Catch2 is already bundled with Minetest, so there are no added dependencies. * Increment failed modules count for Catch2 tests * Respect --test-module option for Catch2 tests * Improve Catch2 --test-module behavior This switches infostream to rawstream so that test runner output is displayed, and returns the correct boolean depending on the results. The tests are now found by setting the configuration instead of invoking the command line parser. * Test uniqueness of all IDS instead of just one Co-Authored-By: Lars Müller * Include Catch2 test run in timing and logging * Flush std::cout after printing Catch results * Increment total tests run instead of hardcoding to 1 * Flush stderr before printing to stdout It's necessary to flush stderr before printing to stdout in adition to flushing stdout before printing to stderr, to make sure all output is ordered correctly. * Make Catch write to rawstream --------- Co-authored-by: Lars Müller --- CMakeLists.txt | 2 +- lib/catch2/CMakeLists.txt | 3 +- src/CMakeLists.txt | 8 +- src/activeobjectmgr.h | 1 - src/unittest/test.cpp | 37 +++++- src/unittest/test_clientactiveobjectmgr.cpp | 119 ++++++++++---------- 6 files changed, 101 insertions(+), 69 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b8254db7f..4c6c60ba9 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -278,7 +278,7 @@ if(NOT USE_LUAJIT) add_subdirectory(lib/bitop) endif() -if(BUILD_BENCHMARKS) +if(BUILD_UNITTESTS OR BUILD_BENCHMARKS) add_subdirectory(lib/catch2) endif() diff --git a/lib/catch2/CMakeLists.txt b/lib/catch2/CMakeLists.txt index 3c471d49d..077272571 100644 --- a/lib/catch2/CMakeLists.txt +++ b/lib/catch2/CMakeLists.txt @@ -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}") diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index bad7e2afb..6ea544980 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -615,8 +615,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) @@ -677,8 +677,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) diff --git a/src/activeobjectmgr.h b/src/activeobjectmgr.h index 0205aee85..369e8acf5 100644 --- a/src/activeobjectmgr.h +++ b/src/activeobjectmgr.h @@ -31,7 +31,6 @@ class TestServerActiveObjectMgr; template class ActiveObjectMgr { - friend class ::TestClientActiveObjectMgr; friend class ::TestServerActiveObjectMgr; public: diff --git a/src/unittest/test.cpp b/src/unittest/test.cpp index 7141ee0c6..2fbba5e4a 100644 --- a/src/unittest/test.cpp +++ b/src/unittest/test.cpp @@ -17,6 +17,11 @@ 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 +// we want to have catch write to rawstream (stderr) instead of stdout +#define CATCH_CONFIG_NOSTDOUT +#include + #include "test.h" #include "nodedef.h" @@ -27,6 +32,21 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "porting.h" #include "debug.h" +#include + +// make catch write everything to rawstream +namespace Catch { + std::ostream& cout() { + return rawstream; + } + std::ostream& clog() { + return rawstream; + } + std::ostream& cerr() { + return rawstream; + } +} + content_t t_CONTENT_STONE; content_t t_CONTENT_GRASS; content_t t_CONTENT_TORCH; @@ -234,6 +254,16 @@ bool run_tests() num_total_tests_run += testmod->num_tests_run; } + rawstream << "Catch test results: " << std::endl; + auto num_catch_tests_failed = Catch::Session().run(); + // We count the all the Catch tests as one test for Minetest's own logging + // because we don't have a way to tell how many individual tests Catch ran. + ++num_total_tests_run; + if (num_catch_tests_failed > 0) { + ++num_modules_failed; + ++num_total_tests_failed; + } + u64 tdiff = porting::getTimeMs() - t1; g_logger.setLevelSilenced(LL_ERROR, false); @@ -269,8 +299,11 @@ bool run_tests(const std::string &module_name) auto testmod = findTestModule(module_name); if (!testmod) { - errorstream << "Test module not found: " << module_name << std::endl; - return 1; + rawstream << "Did not find module, searching Catch tests: " << module_name << std::endl; + Catch::Session session; + session.configData().testsOrTags.push_back(module_name); + auto catch_test_failures = session.run(); + return catch_test_failures == 0; } g_logger.setLevelSilenced(LL_ERROR, true); diff --git a/src/unittest/test_clientactiveobjectmgr.cpp b/src/unittest/test_clientactiveobjectmgr.cpp index d3cd83dc9..2df099fb1 100644 --- a/src/unittest/test_clientactiveobjectmgr.cpp +++ b/src/unittest/test_clientactiveobjectmgr.cpp @@ -17,11 +17,14 @@ 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 #include "test.h" -#include "profiler.h" +#include "client/activeobjectmgr.h" + +#include + +#include +#include class TestClientActiveObject : public ClientActiveObject { @@ -58,9 +61,6 @@ public: void runTests(IGameDef *gamedef); - void testFreeID(); - void testRegisterObject(); - void testRemoveObject(); void testGetActiveSelectableObjects(); }; @@ -68,75 +68,74 @@ static TestClientActiveObjectMgr g_test_instance; void TestClientActiveObjectMgr::runTests(IGameDef *gamedef) { - TEST(testFreeID); - TEST(testRegisterObject) - TEST(testRemoveObject) TEST(testGetActiveSelectableObjects) } //////////////////////////////////////////////////////////////////////////////// -void TestClientActiveObjectMgr::testFreeID() +static TestClientActiveObject* register_default_test_object( + client::ActiveObjectMgr &caomgr) { + auto test_obj = std::make_unique(); + auto result = test_obj.get(); + REQUIRE(caomgr.registerObject(std::move(test_obj))); + return result; +} + +TEST_CASE("test client active object manager") { client::ActiveObjectMgr caomgr; - std::vector aoids; + auto tcao1 = register_default_test_object(caomgr); - 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(); - 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()); + SECTION("When we register many client objects, " + "then all the assigned IDs should be unique.") + { + // This should be enough rounds to be pretty confident + // there are no duplicates. + u16 n = 255; + std::unordered_set ids; + ids.insert(tcao1->getId()); + for (u16 i = 0; i < n; ++i) { + auto other_tcao = register_default_test_object(caomgr); + ids.insert(other_tcao->getId()); + } + // n added objects & tcao1 + CHECK(n + 1 == static_cast(ids.size())); } - caomgr.clear(); -} + SECTION("two registered objects") + { + auto tcao2 = register_default_test_object(caomgr); + auto tcao2_id = tcao2->getId(); -void TestClientActiveObjectMgr::testRegisterObject() -{ - client::ActiveObjectMgr caomgr; - auto tcao_u = std::make_unique(); - auto tcao = tcao_u.get(); - UASSERT(caomgr.registerObject(std::move(tcao_u))); + auto obj1 = caomgr.getActiveObject(tcao1->getId()); + REQUIRE(obj1 != nullptr); - u16 id = tcao->getId(); + auto obj2 = caomgr.getActiveObject(tcao2_id); + REQUIRE(obj2 != nullptr); - auto tcaoToCompare = caomgr.getActiveObject(id); - UASSERT(tcaoToCompare->getId() == id); - UASSERT(tcaoToCompare == tcao); + 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()); + } - tcao_u = std::make_unique(); - tcao = tcao_u.get(); - UASSERT(caomgr.registerObject(std::move(tcao_u))); - UASSERT(caomgr.getActiveObject(tcao->getId()) == tcao); - UASSERT(caomgr.getActiveObject(tcao->getId()) != tcaoToCompare); + SECTION("When we register and query for an object, " + "its memory location should not have changed.") + { + CHECK(obj1 == tcao1); + CHECK(obj2 == tcao2); + } + } - caomgr.clear(); -} - -void TestClientActiveObjectMgr::testRemoveObject() -{ - client::ActiveObjectMgr caomgr; - auto tcao_u = std::make_unique(); - auto tcao = tcao_u.get(); - UASSERT(caomgr.registerObject(std::move(tcao_u))); - - u16 id = tcao->getId(); - UASSERT(caomgr.getActiveObject(id) != nullptr) - - 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(); }