Separating UI from the rest + Unit Testability

[greebo]

I've had some reorganisation work going on the last few weeks, and I thought I'd give a short overview about what changed and why.

Originating from the discussion about lacking unit-testability of DarkRadiant's code, I went ahead and moved all vital DR parts into a shared module. The idea is to load this module from both the main DarkRadiant.exe and the unit testing code. One goal was to enable DR in "headless" mode, so you could run it without any UI, or maybe use a different UI than wxWidgets (theoretically).

After moving most of the modules to the core and getting it to compile and work, I went the extra mile to clip off any UI-related code from the core binary, which was sometimes a bit tricky, but I think I'm there.

From the bird's eye, DR now consist of the following:

• An EXE containing the UI based on wxWidgets, including main(). Almost no map editing logic here.
• The Core / Server DLL containing all modules like VFS, registry, map algorithm, shader, model handling, etc. No wxWidgets allowed here.
• A couple of plugins (as before, nothing has changed)
• A couple of static libraries like scenelib and modulelib, and a few header-only libs

The main() function is still in the EXE, but one of the first jobs is to set up an ApplicationContext and use it to instantiate the core module. This will load the rest of the modules and initialise DarkRadiant's functional part.

All Dialogs, Mouse-Handling, Shortcut Management is handled by the UI binary. When a button is clicked to do something, the UI can communicate with the core module either by the interfaces as defined in the include/i****.h headers or (in case it's a one-way command) by sending commands through the CommandSystem (as with the console commands).

There are cases when the core module needs to request stuff from the UI (if there is one), for example when it needs to ask for a filename and display the FileChooser to the user, or dispatch a command failure (such that an Error Popup can be displayed), or whether it's ok to do an automatic save. For this purpose DR now offers a communication channel named "MessageBus", which can be used to send messages or requests to possible listeners. Some message types might not have any listeners and will stay unhandled, the sending party needs to deal with that. Right now, all message types are subscribed and handled by the UI, but the MessageBus *could* be used for general-purpose communication or event handling too.

Compilation in Windows and Linux is already working. In Linux, there's a libradiantcore.so, in Windows it's DarkRadiantCore.dll. The Xcode project for macOS is not up to date yet.

The next step for me is to merge all this into the codereader/master branch and then merge all the changes which have been pushed to orbweaver/master in the meantime. After that, I'd like to start looking into adding the first unit test based on Google Test, as it seems to be light weight and cross-platform.

[STiFU]

Had a quick look at google Test. Apparently, it is also based on Macros. I've only had experience with boost.Test so far, which also heavily uses macros and frequently confuses Intellisense/Visual Assist (no auto-completion, etc.). But aside from the broken Intellisense, boost.Test is pretty comfortable to work with. 

I'd be interested to hear about your first experiences working with google test and how well Visual Studio (which version?) handles it.

[stgatilov]

For the game we settled on doctest, mostly to avoid potential increase in build time.

Anyway, there is about one unit test in TDM. The main problem is that testing one class from the game or engine is rarely possible. Almost everything relies on global state properly set up. I imagine DarkRadiant has the same problem: everything needs a bunch of global objects to be initialized properly. So I wonder how are you going to write unit tests... Maybe it would be automated tests which test the whole thing at once?

[greebo]

DarkRadiant is much more modular than TDM, we worked for years to get to that point. We don't have that many globals or statics, it's mostly the static module instances which are declared as globals.

The modules do have dependencies, but still it's possible to test their interfaces separately, one after the other. What I'm mostly interested in is not whether GlobalEntityClassManager().findOrCreateEntityDef() is working or not, but whether inserting a new worldspawn in the scenegraph is working properly, resorting the existing entities, or whether the map saving algorithm is working, producing a valid map.

I know that the above are not strictly called unit tests but rather integration tests, but I don't care too much about naming. I need proper machine tests to be more confident when refactoring stuff in those sensitive algorithms. Every time I'm touching the map saving code I feel my neck hair raising because of pure angst.

[STiFU]

2 hours ago, STiFU said:

Had a quick look at google Test. Apparently, it is also based on Macros. I've only had experience with boost.Test so far, which also heavily uses macros and frequently confuses Intellisense/Visual Assist (no auto-completion, etc.). But aside from the broken Intellisense, boost.Test is pretty comfortable to work with. 

I'd be interested to hear about your first experiences working with google test and how well Visual Studio (which version?) handles it.

Just FYI, I just found out that you can either provide hints for visual assist, or enable deep macro parsing (which will slow VA down a bit) to resolve this issue.

Sources:
https://forums.wholetomato.com/forum/topic.asp?TOPIC_ID=8594 (BOOST.TEST)
https://forum.wholetomato.com/forum/topic.asp?TOPIC_ID=9052 (Google Test)

[OrbWeaver]

Sounds pretty good. I look forward to merging all this when it's ready.

[greebo]

On this related note, I got the first integration tests (five of them!) up and running in VC++ at least. Taking this bug https://bugs.thedarkmod.com/view.php?id=5336 as starting point, I created a test that runs into the same problem. After fixing the code the test went to green status, just as the doctor ordered.

Getting the test to run was still quite some work, but now it's possible to write a fixture like this:

TEST_F(RadiantTest, CSGMergeTwoRegularWorldspawnBrushes)
{
    loadMap("csg_merge.map");

    // Locate the first worldspawn brush
    auto worldspawn = GlobalMapModule().getWorldspawn();

    // Try to merge the two brushes with the "1" and "2" materials
    auto firstBrush = test::algorithm::findFirstBrushWithMaterial(worldspawn, "1");
    auto secondBrush = test::algorithm::findFirstBrushWithMaterial(worldspawn, "2");

    ASSERT_TRUE(Node_getIBrush(firstBrush)->getNumFaces() == 5);
    ASSERT_TRUE(Node_getIBrush(secondBrush)->getNumFaces() == 5);

    // Select the brushes and merge them
    GlobalSelectionSystem().setSelectedAll(false);
    Node_setSelected(firstBrush, true);
    Node_setSelected(secondBrush, true);

    // CSG merge
    GlobalCommandSystem().executeCommand("CSGMerge");

    // The two brushes should be gone, replaced by a new one
    ASSERT_TRUE(firstBrush->getParent() == nullptr);
    ASSERT_TRUE(secondBrush->getParent() == nullptr);

    // The merged brush will carry both materials
    auto brushWithMaterial1 = test::algorithm::findFirstBrushWithMaterial(worldspawn, "1");
    auto brushWithMaterial2 = test::algorithm::findFirstBrushWithMaterial(worldspawn, "2");

    ASSERT_TRUE(brushWithMaterial1 == brushWithMaterial2);
    ASSERT_TRUE(Node_getIBrush(brushWithMaterial1)->getNumFaces() == 6);
}

It's more like a script. Before the first line in the test is executed, the whole DR core binary is loaded including all non-UI modules, all modules like VFS, DEF loaders etc. are initialised. Then the above snippet is run, and DR is shut down again afterwards. This test takes around 200ms to perform, which is going to increase once we load more test resources, right now it's just the map file. The test resources are in the repository's test/resources folder, currently it's populated with two .map files designed to work with the CSG merge test cases.

I'm still not 100% sure whether the test code is portable, or if any resources are loaded from my regular TDM installation (I suppose not), I'll fix that when I get to test this in Linux. Google Test is portable, so I should be able to set it up analogously.

Visual Studio integration is nice, the tests are discovered after compilation, and it's possible to select specific or all tests and run/debug them:

[greebo]

I recently finished the test setup, it's working in Linux and Windows now. I migrated almost all the existing tests to gtest, except for the FacePlane class tests which I cannot reach from within the test binary.

If the google test library is available (it's called libgtest-dev in Ubuntu, or gtest in openSUSE), you can run make check to run the test suite. It's only working after make install, not sure if this is usual for make check. You won't see much on the console if the tests are succeeding, but there is a test/drtest.log file which contains the detailed output of the tests.

For simple tests, it's enough to add a .cpp file like this:

#include "gtest/gtest.h"
#include "math/Vector3.h"

namespace test
{

TEST(Vector3, Constructor)
{
    Vector3 vec(1.0, 2.0, 3.5);

    EXPECT_EQ(vec.x(), 1.0);
    EXPECT_EQ(vec.y(), 2.0);
    EXPECT_EQ(vec.z(), 3.5);
}

}

For the more sophisticated tests involving the modules there is a test fixture which can be subclasses to run a specific test. The RadiantTest fixture will boot a headless DarkRadiant instance without any plugins. At the beginning of each test, one has a running DarkRadiant instance with an empty map to work with. Each test can work with a fresh setup, since the environment is torn down after the test code is done:

#include "RadiantTest.h"
#include "ishaders.h"

namespace test
{

using MaterialsTest = RadiantTest;

TEST_F(MaterialsTest, MaterialFileInfo)
{
    auto& materialManager = GlobalMaterialManager();

    // Expect our example material definitions in the ShaderLibrary
    EXPECT_TRUE(materialManager.materialExists("textures/orbweaver/drain_grille"));
}

}