Unittests #7796
Unittests #7796
Conversation
There was some discussion about introducing unit tests to OpenTTD on irc. Here's a proof of concept with some unit tests for a few basic math functions. To run these tests, you need to install [Googletest](https://github.com/google/googletest). On Debian you can do an `apt install googletest`. Then you should be able to run them by going to the `test` directory and running `./test-all.sh`. If we go further down this route we can of course set these up with the CI stuff like Azure.
| Dimension d1 = {0, 0}; | ||
| Dimension d2 = {120, 100}; | ||
|
|
||
| EXPECT_EQ(120, maxdim(d1, d2).width); |
There was a problem hiding this comment.
What actually does "EXPECT_EQ" do, especially if the two expressions don't equal each other?
There was a problem hiding this comment.
EXPECT_* macros are similar to ASSERT_* in the sense that they both test some condition and generate a failure if this condition fails. The difference between ASSERT_* and EXPECT_* is that when using ASSERT_* the function will be aborted on failure, while EXPECT_* will not abort the function. The _EQ part means that it will test if both arguments are equal.
In this specific case, if those expressions do not equal google test will report the test as failed, but the test is not aborted on the failure.
There was a problem hiding this comment.
I do like this, though I (still) have a few general thoughts:
- This needs to be integrated into the build system/CI to be worthwhile at all, otherwise it's just dead code
- I'm not sure I'm a fan of of the *_unittest.cpp files being scattered in amongst the actual source code. It feels a bit messy. Quite happy to be wrong here though.
- Funny you should mention CMake, have you seen Introduce CMake (and removing all other project-related code) #7270 ? :) (At this point, will likely be merged after 1.10 branch point)
|
|
||
| #include <gtest/gtest.h> | ||
|
|
||
| TEST(MaxdimTest, Zero) { |
There was a problem hiding this comment.
I feel like these are (effectively) functions, so the codestyle should be the same - opening brace on a newline
| CPPFLAGS += -isystem $(GTEST_DIR)/include | ||
|
|
||
| # Flags passed to the C++ compiler. | ||
| CXXFLAGS += -g -Wall -Wextra -pthread -std=c++11 |
There was a problem hiding this comment.
Should be pulling these from the main makefile, or be generated with configure, imo
| $(AR) $(ARFLAGS) $@ $^ | ||
|
|
||
|
|
||
| geometry_func.o: $(USER_DIR)/core/geometry_func.cpp $(USER_DIR)/core/geometry_func.hpp $(GTEST_HEADERS) |
There was a problem hiding this comment.
shouldn't we be able to use the same object files from the main build system? Doesn't really make sense to recompile it with a (potentially) different set of defines & flags
|
|
||
| make | ||
|
|
||
| for TEST in geometry_func_unittest math_func_unittest |
There was a problem hiding this comment.
Too hardcoded, should probably be *_uinttest*, much like the gitignore
| $(USER_DIR)/core/math_func.hpp $(GTEST_HEADERS) | ||
| $(CXX) $(CPPFLAGS) $(CXXFLAGS) -c $(USER_DIR)/core/math_func_unittest.cpp | ||
|
|
||
| math_func_unittest: math_func.o math_func_unittest.o $(GTEST_LIBS) |
There was a problem hiding this comment.
Many of these makefile rules are nearly identical - would be good to get some wildcard usage in here to reduce duplication (and effort in adding new unittests)
| @@ -0,0 +1,3 @@ | |||
| [submodule "googletest"] | |||
There was a problem hiding this comment.
This should probably be added at a specific version/tag (and documented as such), rather than whatever was latest
added catch2 header only test framework math tests from OpenTTD#7796
added catch2 header only test framework math tests from OpenTTD#7796
Basically I try to reopen #7254 which is closed. One of the final notes made in that PR is that we cannot have the Makefile rely on system dependent file paths. One of the proposed ways to achieve this is to add googletest as a submodule.
For now, I believe this is the easiest way to achieve the possibility to add unittests to OpenTTD. As I do not see anyone in the near future integrate googletests through autotools, or make OpenTTD use CMake.
I think it is good to have some framework in place to add unittests to OpenTTD. Many things are difficult to test within OpenTTD, but there are some things which you can test, for example #7735 requires some URI parser which should be testable.