lethal-guitar / RigelEngine

A modern re-implementation of the classic DOS game Duke Nukem II
GNU General Public License v2.0
914 stars 60 forks source link

Add unit tests for `ModLibrary` #846

Open lethal-guitar opened 2 years ago

naftalimurgor commented 2 years ago

Hello @lethal-guitar, I would like to take on this, any useful pointers to get started?

Thank you!

lethal-guitar commented 2 years ago

Hey @naftalimurgor, sure thing!

So this is about the class ModLibrary, found here: https://github.com/lethal-guitar/RigelEngine/blob/2dc22c5fd5806283a9dadd95dbffa5915ccae928/src/data/mod_library.hpp#L50

More specifically, the member function rescan of that class. This function looks at the mods directory, lists all the subdirectories it finds, and then merges the result with the previously known list of mods. What complicates things slightly is that we want to preserve any configuration the user has made, e.g. enabling/disabling certain mods as well as ordering mods a certain way. But we also want to deal properly with mod directories being added/deleted.

Further complicating things is the data layout, which is optimized for fast reordering. Instead of storing mod path and enabled state together in a single list, these are split across two lists, with one list containing all the mod paths, and the other list pointing into the first one via indices.

Ok, and now because all of this logic is a bit tricky, it would be nice to have some unit tests for the different cases, like dir list unchanged, mods added only, mods removed only, and mods added and removed at the same time.

Ideally, these unit tests would stub out or mock the part of the code that queries the actual filesystem, and instead allow injecting a fake list of directories for testing. How exactly to do that would be part of this work, as there are various options. I haven't thought too much about this yet, but I could imagine passing in a std::function in the constructor that does the filesystem scanning, or maybe the part of rescan that works with the results of the filesystem scan could be extracted into a free function.

EDIT: Also see https://github.com/lethal-guitar/RigelEngine/wiki/Modding-support#mods-directory for further context.

lethal-guitar commented 2 years ago

Ah, I forgot to mention, you can find existing unit tests here:

https://github.com/lethal-guitar/RigelEngine/tree/master/test

So for this issue here, I would suggest adding a new file called test_mod_library.cpp and implementing the tests there.

The testing framework used is https://github.com/catchorg/Catch2

naftalimurgor commented 2 years ago

@lethal-guitar Thanks, really helpful information. Will submit a PR once I set things up

lethal-guitar commented 2 years ago

Great, sounds good! Let me know if you have any questions.

bencsikandrei commented 7 months ago

and instead allow injecting a fake list of directories for testing. How exactly to do that would be part of this work, as there are various options. I haven't thought too much about this yet, but I could imagine passing in a std::function in the constructor that does the filesystem scanning, or maybe the part of rescan that works with the results of the filesystem scan could be extracted into a free function.

I'm actually looking into trying to make some part of rescan a free function, would make it much easier to test its output (as now it's just a void function) - we can then use the free function inside the mod library class

My initial effort (https://github.com/lethal-guitar/RigelEngine/pull/919) was trying to stub out the filesystem, sadly, even if it was a fun experiment, it's probably far from enough

EDIT: actually maybe I can use the getters to check the result of rescan, TBD