Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unittests #7796

Closed
wants to merge 3 commits into from
Closed

Unittests #7796

wants to merge 3 commits into from

Conversation

@sdcloudt
Copy link
Contributor

sdcloudt commented Oct 22, 2019

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.

nikolas and others added 3 commits Feb 20, 2019
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);

This comment has been minimized.

Copy link
@James103

James103 Oct 22, 2019

Contributor

What actually does "EXPECT_EQ" do, especially if the two expressions don't equal each other?

This comment has been minimized.

Copy link
@sdcloudt

sdcloudt Oct 22, 2019

Author Contributor

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.

Copy link
Member

LordAro left a 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 #7270 ? :) (At this point, will likely be merged after 1.10 branch point)
src/core/geometry_func_unittest.cpp Show resolved Hide resolved

#include <gtest/gtest.h>

TEST(MaxdimTest, Zero) {

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 25, 2019

Member

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

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 25, 2019

Member

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)

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 25, 2019

Member

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

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 25, 2019

Member

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)

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 25, 2019

Member

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"]

This comment has been minimized.

Copy link
@LordAro

LordAro Oct 25, 2019

Member

This should probably be added at a specific version/tag (and documented as such), rather than whatever was latest

@sdcloudt
Copy link
Contributor Author

sdcloudt commented Dec 16, 2019

It is funny how I am coming to the same conclusion as in #7254, namely that it is probably best to wait until cmake (#7270) is ready. I think it is best to close this PR for now.

@sdcloudt sdcloudt closed this Dec 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.