search

MengeCrowdSim / Menge

The source code for the Menge crowd simulation framework
Apache License 2.0
139 stars 64 forks source link

WIP - Adding support to tests #92

Closed mauricioborges closed 6 years ago

mauricioborges commented 6 years ago

Hi there!

In fact I've created this PR more like a way to get in touch with you folks :)

I've started a project of creating a GUI interface for Menge using QT for educational purposes, however I felt that I was not very comfortable with the code yet so I'm creating some characterization tests.

I've used Google Test so far, and I've created a root level CMakeFile.

I need to write some README.md as well

This change is Reviewable

curds01 commented 6 years ago

Thanks for contributing! Always happy to welcome someone to the Menge family.

I'm going to take a look at this later today and I'll get back to you.

curds01 commented 6 years ago

By the way, you might want to check this out:

https://github.com/curds01/MengeConfig

If you want to pool our resources on this, I'd be totally down with that.

curds01 commented 6 years ago

Sorry for the delay -- I keep meaning to get to this. This inspired some house cleaning that ended up larger than I'd expected. This is the very next thing on my todo. I'm eager to start adding unit tests.

MengeCrowdSim commented 6 years ago

This is outstanding work you've done here. The reason Menge hasn't previously had tests is that I'm not overly familiar with cmake and getting tests integrated into the code base. So, I've made a number of questions/comments below exposing that ignorance and hoping to strike up some conversations.

If you don't have ready answers for the questions, that's alright. We can defer the answers as we work with it and figure things out.

I'm so looking forward to getting meaningful testing into Menge. And I'm looking forward to working with you on improving Menge.

(BTW, I've enabled reviewable for Menge. I highly recommend it as a much more amenable mechanism for conducting code reviews. The PR should have a link in the opening comment you can click on to access it.)

Review status: all files reviewed at latest revision, all discussions resolved.

.gitignore, line 1 at r1 (raw file):

cmake-build-debug

Where does this path come from?

.gitignore, line 40 at r1 (raw file):

*.dll

# Fortran module files

Can you explain the origin of all these entries? Some of them seem relevant, some don't belong at all (like the fortran module files). I'd rather not just include things en masse, but make sure that everything added here has an explicit purpose. In other words, if I were to remove a line, I should be affected. And right now, I'm not believing it.

CMakeLists.txt, line 1 at r1 (raw file):

cmake_minimum_required(VERSION 2.8)

I'd like to talk about this file; I've gone out of my way to put all build infrastructure in the projects directory. This has now leaked out. Can we put it in /projects/g++? What I'd prefer is something along the lines of '/projects/g++/testing' (in parallel with /projects/g++/Menge and /projects/g++/Plugins. And then we can simply modify the Makefile in the root to invoke the appropriate cmake apparatus.

That said, I'm open to alternatives. I'd just like to hear the reasons for this file located here.

src/Menge/MengeCore/PluginEngine/ElementDatabase.h, line 188 at r1 (raw file):


  template < class Factory, class Element >
      void ElementDB< Factory, Element >::addBuiltins() {}

Looks like this accidentally crept into the PR. Should probably remove it.

src/test/CMakeLists.txt, line 12 at r1 (raw file):

    gtest
     URL https://github.com/google/googletest/archive/release-1.8.0.zip
#    URL https://github.com/google/googletest/archive/master.zip

BTW We don't want to include commented out lines without some documentation why we're including it. Otherwise, just delete it.

src/test/CMakeLists.txt, line 47 at r1 (raw file):


add_subdirectory(MengeCore)

BTW This is great. Ultimately, the contents of the test directory will mirror that of the Menge directory where tests will be located in a parallel directory from the units they test.

src/test/MengeCore/CMakeLists.txt, line 3 at r1 (raw file):

file(GLOB SRCS *.cpp)

ADD_EXECUTABLE(mengeCoreTest ${SRCS})

I must admit my ignorance. This looks like it'll accumulate all tests into one giant executable. In my limited experience, I've seen each .cpp file go into its own test so that they can be executed more easily in parallel for CI.

Thoughts?

src/test/MengeCore/first.cpp, line 6 at r1 (raw file):

#include "gtest/gtest.h"
#include "gmock/gmock.h"
#include "../../Menge/MengeCore/Runtime/SimulatorDB.h"

I'd much rather see the cmake file modified to include the Menge source directory so that these includes look like:

#include "Menge/MengeCore/Runtime/SimulatorDB.h"
#include "Menge/MengeCore/PluginEngine/CorePluginEngine.h"

src/test/MengeCore/first.cpp, line 9 at r1 (raw file):

#include "../../Menge/MengeCore/PluginEngine/CorePluginEngine.h"
#include "MengeVis/SceneGraph/GLScene.h"

BTW I'm pushing Menge to be compliant with the google styleguide. I have a PR waiting in the wings that will update a lot of the formatting of Menge and include a clang-format configuration. This will probably land first, and I'll just add this file to the PR that updates formatting.

src/test/MengeCore/first.cpp, line 31 at r1 (raw file):


};

These tests should get some // documentation to explain their purpose and, as necessary, how they go about achieving that purpose (if the code doesn't speak clearly for itself).

src/test/MengeCore/first.cpp, line 56 at r1 (raw file):


    const std::string &behaveFile = "/home/mauricio/dev/Menge/examples/core/4square/4squareB.xml";

We'll have to fix these paths. Obviously, these tests won't pass for anyone else.

src/test/MengeCore/first.cpp, line 64 at r1 (raw file):

                                                                     scbVersion, VERBOSE);

    EXPECT_FALSE(sim == 0x00);

We're modernizing Menge's C++ and prefer nullptr to 0x00 going forward.

Comments from Reviewable

mauricioborges commented 6 years ago

hello there! Sorry, I was a bit offline lately, but now I'll jump back here. Soon I'll reply to your comments

mauricioborges commented 6 years ago

Review status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

.gitignore, line 1 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
Where does this path come from?

so, it seems that along the path I've done some tests I created this folder. but I'll remove it

.gitignore, line 40 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
Can you explain the origin of all these entries? Some of them seem relevant, some don't belong at all (like the fortran module files). I'd rather not just include things *en masse*, but make sure that everything added here has an explicit purpose. In other words, if I were to remove a line, I *should* be affected. And right now, I'm not believing it.

I should admit I used the gitignore file that Github has for C++ (https://github.com/github/gitignore/blob/master/C%2B%2B.gitignore). However I agree with your approach, will rebase it and only add the files I have locally (by the way I use Intellij CLion what may have created a different set of files locally)

CMakeLists.txt, line 1 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
I'd like to talk about this file; I've gone out of my way to put all build infrastructure in the projects directory. This has now leaked out. Can we put it in `/projects/g++`? What I'd *prefer* is something along the lines of '/projects/g++/testing' (in parallel with `/projects/g++/Menge` and `/projects/g++/Plugins`. And then we can simply modify the `Makefile` in the root to invoke the appropriate cmake apparatus. That said, I'm open to alternatives. I'd just like to hear the reasons for this file located here.

So, I've created this file here because that's what I'm more used to (in other languages actually, I'm a very beginner in C++). But I can adapt it to use the current pattern, is not a problem for me at all. In fact I've seen both approaches in C++ community but I can't find a pattern). Accepted suggestion indeed, will do that

src/Menge/MengeCore/PluginEngine/ElementDatabase.h, line 188 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
Looks like this accidentally crept into the PR. Should probably remove it.

will do

src/test/MengeCore/CMakeLists.txt, line 3 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
I must admit my ignorance. This looks like it'll accumulate all tests into one giant executable. In my limited experience, I've seen each .cpp file go into its own test so that they can be executed more easily in parallel for CI. Thoughts?

I thought about something like one executable per module (MengeCore i.e.) AND one big one that runs all the tests. I like the idea of having 1 per test file to run it in parallel as well, but do you think we would take a so long time that would make it valuable now? (basically trying to run from hard work here :p)

src/test/MengeCore/first.cpp, line 6 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
I'd much rather see the cmake file modified to include the Menge source directory so that these includes look like: ```c++ #include "Menge/MengeCore/Runtime/SimulatorDB.h" #include "Menge/MengeCore/PluginEngine/CorePluginEngine.h" ```

will do. in fact looks better

src/test/MengeCore/first.cpp, line 31 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
These tests should get some `//` documentation to explain their purpose and, as necessary, how they go about achieving that purpose (if the code doesn't speak clearly for itself).

oh, my mistake. it was actually my way to see how the original method was defined. I can remove this one

Comments from Reviewable

mauricioborges commented 6 years ago

@curds01 about this repository https://github.com/curds01/MengeConfig. Have you built it in VisualStudio? I believe it just works there right?

mauricioborges commented 6 years ago

Review status: 8 of 10 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

src/test/MengeCore/first.cpp, line 9 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
BTW I'm pushing Menge to be compliant with the google styleguide. I have a PR waiting in the wings that will update a lot of the formatting of Menge and include a `clang-format` configuration. This will probably land first, and I'll just add this file to the PR that updates formatting.

awesome!

src/test/MengeCore/first.cpp, line 64 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
We're modernizing Menge's C++ and prefer `nullptr` to `0x00` going forward.

Done.

Comments from Reviewable

mauricioborges commented 6 years ago

Review status: 7 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

src/test/CMakeLists.txt, line 12 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
BTW We don't want to include commented out lines without some documentation *why* we're including it. Otherwise, just delete it.

done

src/Menge/MengeCore/PluginEngine/ElementDatabase.h, line 188 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
will do

done

Comments from Reviewable

MengeCrowdSim commented 6 years ago

Correct; MengeConfig is currently only set up for windows.

mauricioborges commented 6 years ago

Review status: 7 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

CMakeLists.txt, line 1 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
So, I've created this file here because that's what I'm more used to (in other languages actually, I'm a very beginner in C++). But I can adapt it to use the current pattern, is not a problem for me at all. In fact I've seen both approaches in C++ community but I can't find a pattern). Accepted suggestion indeed, will do that

so, in fact I agree on keeping the current structure. I'm quite newbie to cmake and c++ universe (way more used to Java), but I prefer to keep your code style. The only reason I put that there was because it was how I was able to make it work at the time :P. But now I'm back to this effort and I'll refactor it to follow the projects folder pattern

Comments from Reviewable

mauricioborges commented 6 years ago

Review status: 6 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

.gitignore, line 40 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
I should admit I used the gitignore file that Github has for C++ (https://github.com/github/gitignore/blob/master/C%2B%2B.gitignore). However I agree with your approach, will rebase it and only add the files I have locally (by the way I use Intellij CLion what may have created a different set of files locally)

Done.

Comments from Reviewable

mauricioborges commented 6 years ago

Review status: 6 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

src/test/MengeCore/first.cpp, line 31 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
oh, my mistake. it was actually my way to see how the original method was defined. I can remove this one

Done.

Comments from Reviewable

mauricioborges commented 6 years ago

Just added some travis-ci support. I decided for that because circleci doesn't support C++ by default: https://travis-ci.org/mauricioborges/Menge

mauricioborges commented 6 years ago

Review status: 1 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

src/test/CMakeLists.txt, line 47 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
BTW This is great. Ultimately, the contents of the `test` directory will mirror that of the `Menge` directory where tests will be located in a parallel directory from the units they test.

I've seen you done the same for the source code as well. Neat!

Comments from Reviewable

mauricioborges commented 6 years ago

Review status: 1 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

src/test/MengeCore/first.cpp, line 6 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
will do. in fact looks better

Done.

Comments from Reviewable

mauricioborges commented 6 years ago

Review status: 1 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim, @mauricioborges, and @curds01)

src/test/MengeCore/first.cpp, line 56 at r1 (raw file):

Previously, MengeCrowdSim (Menge crowd simulation) wrote…
We'll have to fix these paths. Obviously, these tests won't pass for anyone else.

those paths were actually part of a very flaky test I was playing with. Just removed it.

Comments from Reviewable

mauricioborges commented 6 years ago

Review status: 1 of 9 files reviewed, 9 unresolved discussions (waiting on @MengeCrowdSim and @curds01)

src/test/MengeCore/first.cpp, line 56 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
those paths were actually part of a very flaky test I was playing with. Just removed it.

Done.

CMakeLists.txt, line 1 at r1 (raw file):

Previously, mauricioborges (Mauricio Borges Silva) wrote…
so, in fact I agree on keeping the current structure. I'm quite newbie to cmake and c++ universe (way more used to Java), but I prefer to keep your code style. The only reason I put that there was because it was how I was able to make it work at the time :P. But now I'm back to this effort and I'll refactor it to follow the `projects` folder pattern

Done.

Comments from Reviewable

mauricioborges commented 6 years ago

awesome! hey, what about creating the travis-ci configuration when we have some tests? I'll start creating some tests soon.

MengeCrowdSim commented 6 years ago

I was thinking the same -- since you've created the link on the readme; it's about time to connect it.