Jump to content
The Dark Mod Forums

DarkRadiant 2.8.0 pre-release testing


Recommended Posts

New pre5 is available, sorry for the inconvenience.

It's definitely enough to remove those sections, you should be fine after doing that.

I'll finally look into getting some unit tests done for the next release. These reocurring bugs in the saving and loading algorithms are continuously giving me the creeps, and they are entirely avoidable.

Link to comment
Share on other sites

On 4/26/2020 at 5:28 AM, greebo said:

I'll finally look into getting some unit tests done for the next release. These reocurring bugs in the saving and loading algorithms are continuously giving me the creeps, and they are entirely avoidable.

On that subject, I added some basic unit tests (using Boost.Test) last year when I was working on the asset deprecation functionality, which you might find useful as a basis or guide to implementing new unit tests for saving and loading.

The Boost macros and test fixture system is quite nice to use; the challenge of course is being able to split up the code so that it can be tested in isolation without needing to pull in dozens of other module dependencies which don't work in the test environment.

Link to comment
Share on other sites

Yep, I noticed that, and I welcome every effort getting unit test into the picture. :)

I recently tried to get some tests done validating the save/load process. It turned out that even the "simplest" operations in DarkRadiant rely on a fully loaded module network, hardly anything can be extracted and tested on its own. I went on trying to get some modules set up, which led me to the second problem, that DR is only capable of starting up when run by the main executable - too many statics and other things only working in the main exe.

I'm currently on a branch changing that architecture, such that the core DR functionality is extracted from the main binary into a separate module. This module should be instantiable from both the EXE as well as a unit test module - the only thing required will be an implementation of the ApplicationContext interface.

After that step is done, I'll try to get some unit tests running against test resources that are not relying on having the whole TDM game lying around.

If I'm successful with all that (which is not guaranteed) I'd like to see it taken further such that DarkRadiant's UI is separated from the map editing logic, but that is even further stretched out than the previous goal, so let's see how it works out step by step.

Speaking of unit test frameworks, I'll see what Boost Test can do in comparison to Google Test (which has a smaller footprint than the whole boost library tree). There are test plugins available for Visual Studio for both of them, so I guess both offer the same convenience for me as a primary VC++ user.

Link to comment
Share on other sites

15 hours ago, greebo said:

I recently tried to get some tests done validating the save/load process. It turned out that even the "simplest" operations in DarkRadiant rely on a fully loaded module network, hardly anything can be extracted and tested on its own. I went on trying to get some modules set up, which led me to the second problem, that DR is only capable of starting up when run by the main executable - too many statics and other things only working in the main exe.

Right. That was something I noticed early on in my experiments with testing: trying to set up actual modules is basically impossible because of the complexity and interdependence.

15 hours ago, greebo said:

I'm currently on a branch changing that architecture, such that the core DR functionality is extracted from the main binary into a separate module. This module should be instantiable from both the EXE as well as a unit test module - the only thing required will be an implementation of the ApplicationContext interface.

That is not a bad approach, and might be the design which presents the maximum possible testing functionality. However, before committing to such a large task, it would be worth considering whether there are simpler approaches which allow you to test an isolated class without needing a complete module system.

The problem is that a particular class might call GlobalFoo() which returns a module which then need to instantiate GlobalBar() and everything else. But there are sometimes refactorings you can make which might improve this, for example:

  1. Does the class really need the whole of GlobalFoo(), or is it just looking up some data (like "which image file extensions can I load?"). If it just looking for some simple data, the class under test could be refactored to accept this data as a construction parameter, rather than calling GlobalFoo() and getting the data itself. You can then pass in the data explicitly in the unit test without needing the Foo module to exist.
  2. Does the class only need to call GlobalFoo() for certain operations (like OpenGL rendering), which you are not actually testing? If so, the usage of GlobalFoo could be wrapped in a callback or std::async so it doesn't actually get called until the relevant operation is performed.
  3. Could a mock Foo module be created in the test suite, that performs much simpler operations and/or doesn't rely on any other modules?

I'm sure I used all of these techniques when testing the PK4 file parsing, although I appreciate that this is a much simpler part of DarkRadiant than testing the whole map loading system which undoubtedly relies on loads of other modules for loading various assets.

15 hours ago, greebo said:

Speaking of unit test frameworks, I'll see what Boost Test can do in comparison to Google Test (which has a smaller footprint than the whole boost library tree). There are test plugins available for Visual Studio for both of them, so I guess both offer the same convenience for me as a primary VC++ user.

No objections from me. I did briefly look at Google Test as I recall, and didn't notice any specific problems with it; I think I went with Boost.Test mainly for convenience because we are already heavily using Boost. But if you find Google Test provides better functionality then I don't see a problem with integrating it.

Link to comment
Share on other sites

 

3 hours ago, OrbWeaver said:

Does the class really need the whole of GlobalFoo(), or is it just looking up some data (like "which image file extensions can I load?"). If it just looking for some simple data, the class under test could be refactored to accept this data as a construction parameter, rather than calling GlobalFoo() and getting the data itself. You can then pass in the data explicitly in the unit test without needing the Foo module to exist.

There are a lot of details that make it so hard, like the following snippet (creating a BrushNode is something our map parser would do):

BrushNode::BrushNode() :
    scene::SelectableNode(),
    m_lightList(&GlobalRenderSystem().attachLitObject(*this)),
    m_brush(*this),
    _selectedPoints(GL_POINTS),
    _faceCentroidPointsCulled(GL_POINTS),
    m_viewChanged(false),
    _renderableComponentsNeedUpdate(true),
    _untransformedOriginChanged(true)
{
    m_brush.attach(*this); // BrushObserver

    SelectableNode::setTransformChangedCallback(std::bind(&BrushNode::lightsChanged, this));
}

Notice the GlobalRenderSystem() call right in the constructor... that's not saying this cannot be refactored or changed (in fact the whole LitObject approach needs some love), but it's probably taking so much time that I get lost in the process before I get to write the first unit test. The GlobalRenderSystem() above needs the GlobalOpenGL() module, which in turn needs to be properly initialised, otherwise the shaders cannot be realised. It's likely possible to go without realised shaders for this particular test, but texdef manipulation is something I want to be able to test too...

Another thought I had is that I don't want the test environment differ too much from the production setup, ideally it should be identical.

Modules can indeed be mocked, for some modules this might actually be not that hard. Main downside I see with this approach is that there are so many modules, the decision which modules need to be mocked in a particular unit test might differ for every single test setup (which scared me off).

So I figured we'd get more unit tests faster when we just leave the DR module setup as it is, but make it accessible from not just the main exe. Refactoring the modules is still possible and needs to continue for the next couple of years at least. :D

Another problem I'm yet to face is the game resources used for the automated tests. This might be harder than I want it to be, extracting the necessary resources to get DR in a working state such that it can be initialised and tested. My hope is that I can get away with a small 1-2 MB PK4 containing the necessary stuff, but I didn't cross that bridge yet.

As for the ongoing refactoring, I've already made some solid progress on separating the stuff (only in Windows at the moment, but I think I know how to get the Makefiles in a proper state too). I already have a core module containing the ModuleRegistry, the Logging and the XMLRegistry, and DR is starting up fine so far. Now I plan to move over those modules that are not related to UI step-by-step.

Link to comment
Share on other sites

20 hours ago, greebo said:

Notice the GlobalRenderSystem() call right in the constructor... that's not saying this cannot be refactored or changed (in fact the whole LitObject approach needs some love), but it's probably taking so much time that I get lost in the process before I get to write the first unit test. The GlobalRenderSystem() above needs the GlobalOpenGL() module, which in turn needs to be properly initialised, otherwise the shaders cannot be realised. It's likely possible to go without realised shaders for this particular test, but texdef manipulation is something I want to be able to test too...

Dependencies on the render system in particular are one of the things which I would suggest decoupling is quite important, because I know from experience at work that unit tests which rely on rendering are the most likely to break, especially if they need to run as part of a build process in which full access to the graphical environment may not be available.

With regard to this specific example, a couple of options spring to mind:

  1. Although the m_lightList pointer is initialised in the constructor, it isn't actually used. The constructor could instead initialise the pointer to nullptr, then all other usages of the pointer elsewhere in the class could be replaced by a call to an internal method getLightList() which contains the initialisation code currently implemented in the constructor. This is a case of "kicking the can down the road" in that it doesn't actually remove the dependency on the render system, but delays the call until the result is actually needed (which it might never be, in the unit test).
  2. Instead of calling GlobalRenderSystem() itself, the BrushNode constructor could accept a reference to an abstract RenderSystem interface object, which in the unit test is just a mock object implementing the interface but not doing anything, rather than the real implementation used in DarkRadiant. If RenderSystem is too large and complex to mock in this way, the interface could be split up: a new interface LightListProvider could provide just the virtual methods used here (e.g. attachLitObject()), and this new interface passed to the BrushNode constructor instead of the whole RenderSystem (of course the RenderSystem would also need to be modified to derive from LightListProvider as well).
20 hours ago, greebo said:

Another thought I had is that I don't want the test environment differ too much from the production setup, ideally it should be identical.

Modules can indeed be mocked, for some modules this might actually be not that hard. Main downside I see with this approach is that there are so many modules, the decision which modules need to be mocked in a particular unit test might differ for every single test setup (which scared me off).

It's worth pointing out that there is a difference between a unit test, which is generally understood as an isolated test of just a single class (maybe even a single method), and an integration test which is a much broader test of a large number of subsystems at once. Both types of test are useful: a unit test reveals specific problems in methods or classes which are localised to those methods or classes (and more easily fixed), whereas an integration test will confirm that "everything is [not] working" but probably won't offer much information as to exactly where a problem lies.

I think what you're aiming for here is much closer to an integration test than a unit test. There is nothing wrong with this strategy, which will certainly be useful for confirming that saving code hasn't been broken by some refactoring or change, but if you really want to be able to isolate particular problems with automated tests (i.e. "this method of BrushNode is returning a wrong value", rather than "saving is broken"), then more granular unit tests are ultimately going to be necessary, and unfortunately this does mean that a different set of dependencies may need to be mocked in each unit test. However, the test fixture system can help with this, by allowing sets of mocked dependencies can be re-used across multiple tests, so you don't need to implement them from scratch each time.

20 hours ago, greebo said:

Another problem I'm yet to face is the game resources used for the automated tests. This might be harder than I want it to be, extracting the necessary resources to get DR in a working state such that it can be initialised and tested. My hope is that I can get away with a small 1-2 MB PK4 containing the necessary stuff, but I didn't cross that bridge yet.

Yes, that will almost certainly be necessary, and I did this for the PK4 testing. In my case this was fairly easy but it was for a very simple test setup: basically just checking that a model file would appear with a "hidden" property if there was an assets.lst file in the appropriate place. For testing full-scale saving and loading I suspect a lot more resources will be needed, included several entity definitions, materials and objects, but probably nothing very large in terms of megabytes. Even if you need actual texture images, these could be very low resolution e.g. 16x16 TGAs which would take up very little space in the repository. I don't suppose you will need any sound files and everything else should compress pretty well in the PK4.

Link to comment
Share on other sites

Yes, something lazy-acquiring the render-resources will give us enough room to breath to set up a test for a BrushNode or a brush parser.

You're of course right with the distinction between unit and integration tests. I'm rather sloppy when it comes to using these terms (people call me out on this at work for all the time too) - I guess it's because for me there's not too much of a difference in practice. Unit tests and integration tests are mixed together into the same test assembly or test compilation unit, and both are run by the same testing framework. I didn't often encounter 100% pure unit tests that are only focusing on one single interface either, the test-worthy things (those that caused trouble in the past, usually) are generally covered by integration tests. But that shouldn't be an excuse for sloppy term usage on my behalf. :D

I managed to make some more progress today, and I'm at the point where it's justified to briefly look into the Linux compilation, just to see whether that module separation works as I planned in the gcc binaries. I'm pretty positive though, there's no hacks or over-the-top-latest-C++ standard code involved.

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


  • Recent Status Updates

    • Petike the Taffer

      I've finally managed to log in to The Dark Mod Wiki. I'm back in the saddle and before the holidays start in full, I'll be adding a few new FM articles and doing other updates. Written in Stone is already done.
      · 1 reply
    • nbohr1more

      TDM 15th Anniversary Contest is now active! Please declare your participation: https://forums.thedarkmod.com/index.php?/topic/22413-the-dark-mod-15th-anniversary-contest-entry-thread/
       
      · 0 replies
    • JackFarmer

      @TheUnbeholden
      You cannot receive PMs. Could you please be so kind and check your mailbox if it is full (or maybe you switched off the function)?
      · 1 reply
    • OrbWeaver

      I like the new frob highlight but it would nice if it was less "flickery" while moving over objects (especially barred metal doors).
      · 4 replies
    • nbohr1more

      Please vote in the 15th Anniversary Contest Theme Poll
       
      · 0 replies
×
×
  • Create New...