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

Codechange: introduce a few unit tests #7254

Closed
wants to merge 1 commit into from
Closed

Conversation

@nikolas
Copy link
Member

@nikolas nikolas commented 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. 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.

@nikolas nikolas force-pushed the unittests branch 2 times, most recently from 5ec75f8 to 5ce3214 Feb 20, 2019
src/core/geometry_func_unittest.cpp Outdated Show resolved Hide resolved
@nikolas nikolas force-pushed the unittests branch 3 times, most recently from 6ab0ed9 to 62f4d71 Feb 21, 2019
# make clean - removes all files generated by make.

# Points to the root of Google Test source
GTEST_DIR = /usr/src/googletest/googletest
Copy link
Contributor

@nielsmh nielsmh Feb 21, 2019

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.

Copy link
Member Author

@nikolas nikolas Feb 22, 2019

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?

Copy link
Contributor

@glx22 glx22 Feb 22, 2019

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

Copy link
Member

@LordAro LordAro left a comment

As others have previously mentioned, this should use the existing configure script, and generate a top-level Makefile as appropriate, probably Makefile.test.in

src/core/geometry_func_unittest.cpp Outdated Show resolved Hide resolved
@@ -45,7 +45,8 @@ int GreatestCommonDivisor(int a, int b)
b = a % b;
a = t;
}
return a;
/* GCD is always positive */
return abs(a);
Copy link
Member

@LordAro LordAro Feb 23, 2019

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?

Copy link
Member Author

@nikolas nikolas Feb 25, 2019

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.

Copy link
Contributor

@Eddi-z Eddi-z Feb 25, 2019

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

test/.gitignore Outdated Show resolved Hide resolved
$(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)
Copy link
Member

@LordAro LordAro Feb 23, 2019

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
Copy link
Member

@LordAro LordAro Feb 23, 2019

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
Copy link
Member

@LordAro LordAro Feb 23, 2019

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

@LordAro LordAro added the wip label Feb 23, 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.
@nikolas
Copy link
Member Author

@nikolas nikolas commented Feb 24, 2019

Hmm, the recommended way of integrating Googletest into the build system is actually to use CMake, as configure can't detect where Googletest is installed, as it's not a pre-compiled library.

Projects that integrate Googletest with autotools need to resort to one of these options, as far as I can tell:

  • Download the GoogleTest source code manually and place it at a known location. (what I've done here)
  • Embed the GoogleTest source code as a direct copy in the main project's source tree.
  • Add GoogleTest as a git submodule or equivalent.

None of those are particularly nice solutions. A fourth option lets CMake download and configure Googletest, like this:

cmake_minimum_required(VERSION 2.8.2)

project(googletest-download NONE)

include(ExternalProject)
ExternalProject_Add(googletest
  GIT_REPOSITORY    https://github.com/google/googletest.git
  GIT_TAG           master
  SOURCE_DIR        "${CMAKE_CURRENT_BINARY_DIR}/googletest-src"
  BINARY_DIR        "${CMAKE_CURRENT_BINARY_DIR}/googletest-build"
  CONFIGURE_COMMAND ""
  BUILD_COMMAND     ""
  INSTALL_COMMAND   ""
  TEST_COMMAND      ""
)

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.

@nikolas
Copy link
Member Author

@nikolas nikolas commented Feb 25, 2019

I'm closing this until I integrate Googletest with either autotools or CMake.

@nikolas nikolas closed this Feb 25, 2019
@sdcloudt sdcloudt mentioned this pull request Oct 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants