dfhack icon indicating copy to clipboard operation
dfhack copied to clipboard

unit tests: Add CTest support, and a trivial first unit test

Open softmoth opened this issue 2 years ago • 9 comments

If BUILD_TESTS=ON:

  • Adds a 'test' target for ninja
  • Adds a library/MiscUtils.test unit test executable

This uses the CTest tool that is part of CMake. Hopefully it might provide a basis for unit tests. I tried to do the simplest thing that works for this first commit. CMake has features to integrate Boost.Test or Google Test frameworks, but I'm not sure if that's the direction to go in.

Questions

  • CTest creates a BUILD_TESTING option (on by default) that users can turn off to disable tests. This makes an awkward clash with our BUILD_TESTS option which is meant to do the same thing (governs our Lua integration tests). How should this be handled? I propose removing BUILD_TESTS and using the CMake-standard BUILD_TESTING option for both. If there's a way to make a hidden CMake option BUILD_TESTS that sets BUILD_TESTING for backwards compatibility, that should be added as well.

Flaws in this Draft

  • Dependencies are broken. This shows up on Windows builds.
  • Dependencies are broken. The automatically-generated test target does not depend on executables added with add_test. This is an obvious requirement, so I must be calling things incorrectly.
  • Missing documentation. If this PR solidifies into something that can be merged, I'll add docs.

Follow-up improvements needed

  • Integration with CI and github commit checks.
  • Writing unit tests by hand is not a long-term solution; we'll want to use Boost.Test or Google Test or something.

What do you think?

softmoth avatar Apr 21 '22 03:04 softmoth

I forgot to mention: a problem I'm having is that ninja test will try to run even if ninja hasn't completed yet. I can't figure out how to make it build first before it runs ctest. The docs for add_dependency() make it sound like it's not helpful, but I don't know what else to use.

softmoth avatar Apr 21 '22 03:04 softmoth

This definitely has potential. The cpp code is very ripe for testing.

CTest seems perfectly fine for running the tests, but I think we need a better framework for writing the tests. How difficult would it be to integrate something like Google Test and Google Mock? https://chromium.googlesource.com/external/github.com/google/googletest/+/refs/tags/release-1.8.0/googlemock/docs/ForDummies.md

myk002 avatar Apr 21 '22 04:04 myk002

How difficult would it be to integrate something like Google Test

CTest has built-in support for Boost.Test and Google Test. I thought I could work on that once I get a bare-bones first test or two going.

softmoth avatar Apr 21 '22 13:04 softmoth

Copied from discord so the questions don't get lost:

So I guess there's a number of design decisions we need to make around cpp tests. Should they be 1 binary per test or compiled into a monolithic suite? Should we even have ctest integration, or should the test interface be a plugin (to support integration tests)? Should we have both, one for pure unit tests and the other for integration tests? Is it better/easier to test all cpp code from Lua with the existing test harness?

Also, how will we measure and report cpp code coverage in each of the above scenarios? I have some initial thoughts on this at #2036

myk002 avatar Apr 27 '22 00:04 myk002

To answer your question in the description, it should be fine to replace BUILD_TESTS with BUILD_TESTING, but they'd need to exist in parallel at least for a while until CI is migrated. What do you think about my design questions in the previous comment?

myk002 avatar Apr 30 '22 03:04 myk002

I'm not sure we need to decide up front whether it's monolithic or not. If we're using gtest + CTest, I think we can combine as many tests as makes sense into feature-specific or module-specific test binaries, for example. CTest will run whatever tests we make and automatically enumerate individual cases within.

IMO integration tests should be separate, done via lua. By "integration tests" I mean tests that run Dwarf_Fortress and exercise the entire system. That's already working great with lua, no need to do it in C++ as well. But for testing isolated C++ components, and for enabling test-driven design and safer refactoring, etc., unit tests in C++ are useful.

softmoth avatar Apr 30 '22 20:04 softmoth

I'm not sure we need to decide up front whether it's monolithic or not. If we're using gtest + CTest, I think we can combine as many tests as makes sense into feature-specific or module-specific test binaries, for example. CTest will run whatever tests we make and automatically enumerate individual cases within.

IMO integration tests should be separate, done via lua. By "integration tests" I mean tests that run Dwarf_Fortress and exercise the entire system. That's already working great with lua, no need to do it in C++ as well. But for testing isolated C++ components, and for enabling test-driven design and safer refactoring, etc., unit tests in C++ are useful.

SGTM. and I agree about using Lua for integration tests. I just wanted to know if you had any specific ideas/plans in that area.

myk002 avatar May 09 '22 03:05 myk002

bumping from project for now since the review isn't active. Once you're ready to take this up again, we can look into getting it merged!

myk002 avatar Jun 06 '22 23:06 myk002

I might also be interested in taking a stab at this if that would be helpful

lethosor avatar Jun 10 '22 02:06 lethosor