-
-
Notifications
You must be signed in to change notification settings - Fork 945
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
Codechange: introduce a few unit tests #7254
Conversation
5ec75f8
to
5ce3214
Compare
6ab0ed9
to
62f4d71
Compare
# make clean - removes all files generated by make. | ||
|
||
# Points to the root of Google Test source | ||
GTEST_DIR = /usr/src/googletest/googletest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need to be detected by configure
, can't have it hardcoded like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely... do you think there should be a separate configure
here in the test
directory, or that this Makefile should be generated by the main configure
, behind some flag that isn't enabled by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the usual with-XXX autodetection yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As others have previously mentioned, this should use the existing configure script, and generate a top-level Makefile as appropriate, probably Makefile.test.in
@@ -45,7 +45,8 @@ int GreatestCommonDivisor(int a, int b) | |||
b = a % b; | |||
a = t; | |||
} | |||
return a; | |||
/* GCD is always positive */ | |||
return abs(a); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this is the correct solution - maybe there should be asserts at the beginning that check for non negative numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you intend to limit the domain to positive numbers, then absolutely we could add some asserts at the beginning.
It seems that calculating the GCD of negative numbers in general does produce a positive result:
$ python3
Python 3.7.2+ (default, Feb 2 2019, 14:31:48)
[GCC 8.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import math
>>> math.gcd(-10, -25)
5
>>> math.gcd(10, -25)
5
Makes sense intuitively, since you can divide -25 by 5 without a remainder, and 5 > -5 of course.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be correct, as if n divides both a and b, then -n does as well.
however, i would probably put the abs() at the beginning of the function to avoid any odd side effects in the algorithm
$(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of hardcoding and duplication here, should be much more wildcard usage, and probably use of MAKEDEPEND
|
||
# All tests produced by this Makefile. Remember to add new tests you | ||
# created to the list. | ||
TESTS = geometry_func_unittest math_func_unittest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me like all files ending in _uinttest.cpp
should be used, meaning that there's no need to touch the makefile
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use same set of CFLAGS as the main program
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.
Hmm, the recommended way of integrating Googletest into the build system is actually to use CMake, as Projects that integrate Googletest with autotools need to resort to one of these options, as far as I can tell:
None of those are particularly nice solutions. A fourth option lets CMake download and configure Googletest, like this:
This is all info from here, btw: https://github.com/google/googletest/tree/master/googletest#incorporating-into-an-existing-cmake-project So, as it stands, if we do want to integrate Googletest with autotools, we can either use a git submodule or include Googletest's source with OpenTTD. If anyone thinks those are good ideas I can move forward with that. Otherwise maybe it's better to wait on this until TrueBrain converts the build system over to CMake. |
I'm closing this until I integrate Googletest with either autotools or CMake. |
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. On Debian you can do an
apt install googletest
. Then you should be able to run them by going to thetest
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.