Upgrade client active object mgr tests to Catch2 (#14565)

* 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 <appgurulars@gmx.de>

* 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 <appgurulars@gmx.de>

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()
 

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}")

src/CMakeLists.txt

@@ -615,8 +615,8 @@ if(BUILD_CLIENT)
 	if (USE_SPATIAL)
 		target_link_libraries(${PROJECT_NAME} ${SPATIAL_LIBRARY})
 	endif()
-	if(BUILD_BENCHMARKS)
+	if(BUILD_UNITTESTS OR BUILD_BENCHMARKS)
-		target_link_libraries(${PROJECT_NAME} catch2)
+		target_link_libraries(${PROJECT_NAME} Catch2::Catch2)
 	endif()
 endif(BUILD_CLIENT)
 
@@ -677,8 +677,8 @@ if(BUILD_SERVER)
 			${CURL_LIBRARY}
 		)
 	endif()
-	if(BUILD_BENCHMARKS)
+	if(BUILD_UNITTESTS OR BUILD_BENCHMARKS)
-		target_link_libraries(${PROJECT_NAME}server catch2)
+		target_link_libraries(${PROJECT_NAME}server Catch2::Catch2)
 	endif()
 endif(BUILD_SERVER)
 

src/activeobjectmgr.h

@@ -31,7 +31,6 @@ class TestServerActiveObjectMgr;
 template <typename T>
 class ActiveObjectMgr
 {
-	friend class ::TestClientActiveObjectMgr;
 	friend class ::TestServerActiveObjectMgr;
 
 public:

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 <catch.hpp>
+
 #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 <iostream>
+
+// 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;
+		rawstream << "Did not find module, searching Catch tests: " << module_name << std::endl;
-		return 1;
+		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);

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 <algorithm>
 #include "test.h"
 
-#include "profiler.h"
+#include "client/activeobjectmgr.h"
+
+#include <catch.hpp>
+
+#include <unordered_set>
+#include <utility>
 
 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) {
-	client::ActiveObjectMgr caomgr;
+	auto test_obj = std::make_unique<TestClientActiveObject>();
-	std::vector<u16> aoids;
+	auto result = test_obj.get();
-
+	REQUIRE(caomgr.registerObject(std::move(test_obj)));
-	u16 aoid = caomgr.getFreeId();
+	return result;
-	// 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();
+TEST_CASE("test client active object manager")
-}
-
-void TestClientActiveObjectMgr::testRegisterObject()
 {
 	client::ActiveObjectMgr caomgr;
-	auto tcao_u = std::make_unique<TestClientActiveObject>();
+	auto tcao1 = register_default_test_object(caomgr);
-	auto tcao = tcao_u.get();
-	UASSERT(caomgr.registerObject(std::move(tcao_u)));
 
-	u16 id = tcao->getId();
+	SECTION("When we register many client objects, "
-
+			"then all the assigned IDs should be unique.")
-	auto tcaoToCompare = caomgr.getActiveObject(id);
+	{
-	UASSERT(tcaoToCompare->getId() == id);
+		// This should be enough rounds to be pretty confident
-	UASSERT(tcaoToCompare == tcao);
+		// there are no duplicates.
-
+		u16 n = 255;
-	tcao_u = std::make_unique<TestClientActiveObject>();
+		std::unordered_set<u16> ids;
-	tcao = tcao_u.get();
+		ids.insert(tcao1->getId());
-	UASSERT(caomgr.registerObject(std::move(tcao_u)));
+		for (u16 i = 0; i < n; ++i) {
-	UASSERT(caomgr.getActiveObject(tcao->getId()) == tcao);
+			auto other_tcao = register_default_test_object(caomgr);
-	UASSERT(caomgr.getActiveObject(tcao->getId()) != tcaoToCompare);
+			ids.insert(other_tcao->getId());
-
+		}
-	caomgr.clear();
+		// n added objects & tcao1
+		CHECK(n + 1 == static_cast<u16>(ids.size()));
 	}
 
-void TestClientActiveObjectMgr::testRemoveObject()
+	SECTION("two registered objects")
 	{
-	client::ActiveObjectMgr caomgr;
+		auto tcao2 = register_default_test_object(caomgr);
-	auto tcao_u = std::make_unique<TestClientActiveObject>();
+		auto tcao2_id = tcao2->getId();
-	auto tcao = tcao_u.get();
-	UASSERT(caomgr.registerObject(std::move(tcao_u)));
 
-	u16 id = tcao->getId();
+		auto obj1 = caomgr.getActiveObject(tcao1->getId());
-	UASSERT(caomgr.getActiveObject(id) != nullptr)
+		REQUIRE(obj1 != nullptr);
 
-	caomgr.removeObject(tcao->getId());
+		auto obj2 = caomgr.getActiveObject(tcao2_id);
-	UASSERT(caomgr.getActiveObject(id) == nullptr)
+		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);
+		}
+	}
+
+	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();
 }