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

Adding tests #3

Merged
merged 14 commits into from
Dec 12, 2015
Merged

Conversation

jvarley
Copy link
Member

@jvarley jvarley commented Nov 4, 2015

THIS IS NOT READY TO BE MERGED

I started putting together a test framework, I wanted to make a PR so that you guys could comment, and if you don't think this is a reasonable approach, I would rather know now. I am using gtest.

tests are run with:
qmake tests.pro
make
export GRASPIT=$PWD
./bin/test

I still need to fix this to wait until after testGrasps finishes to call showGrasps, and then get everything to shutdown correctly. So this test currently passes, but is not testing what I want it to test.

@mateiciocarlie
Copy link
Member

This looks like a good start... Thanks for putting it together. My questions would be:

  • how do we feel about gtest as our general test framework? Can you apt-get install it on Ubuntu? Is it available on Windows as well?
  • how would it integrate for example with Travis? I would expect Travis to play nicely with gtest, but don't know for sure...
  • I would prefer not to have duplication between test.pro and graspit.pro... Anyway we can avoid that? Perhaps a flag in graspit.pro that decides if we also build the tests or not.

@jon-weisz - any thoughts?

@jvarley
Copy link
Member Author

jvarley commented Nov 21, 2015

Hi Matei and Jon,

I have a working test case that starts the planner on the mug, and then asserts that a grasp is found for the mug after the planner runs.

To answer your questions Matei:

  1. how do we feel about gtest as our general test framework? Can you apt-get install it on Ubuntu? Is it available on Windows as well?

You can apt-get it, but must still run cmake to build it, it is a bit weird
http://www.eriksmistad.no/getting-started-with-google-test-on-ubuntu/

It appears to work with Windows, although I have not tried it,

  1. how would it integrate for example with Travis? I would expect Travis to play nicely with gtest, but don't know for sure...

several demos exist showing integration
https://github.com/bast/gtest-demo

  • I would prefer not to have duplication between test.pro and graspit.pro... Anyway we can avoid that? Perhaps a flag in graspit.pro that decides if we also build the tests or not.

This still has two separate .pro files for graspit-test and and graspit, if we want to have a single top level .pro file, then we need to use qt subdirs which would require a bit of rearranging of the project which I am not sure is worth it. Basically we would have to put all the graspit code in one subdirectory, and then all the test code in a parallel subdirectory. Then we would have a top level .pro file, I am concerned this may do weird things with previous documentation about how $GRASPIT is defined.

tests are run with:
qmake graspit-test.pro
make
export GRASPIT=$PWD
./bin/graspit-test

@jvarley
Copy link
Member Author

jvarley commented Nov 21, 2015

Honestly, it looks like the easiest fix to get a single file to build both tests and graspit, would be to switch to cmake, this would make it easy to include gtest as a submodule as well following this example:

https://github.com/bast/gtest-demo

I have been writing a graspit plugin that has a few .ui files of its own, and calling the respective qmake commands from cmake is pretty easy.
https://github.com/CURG/graspit_bci_plugin/blob/master/CMakeLists.txt

But none of this is needed for this PR.

@jvarley
Copy link
Member Author

jvarley commented Dec 10, 2015

This is ready to be pulled in. I would like to pull it in soon so that as I make other changes, I can write tests for them. Let me know if you have any problems with the current implementation and I can fix/address them.

@mateiciocarlie
Copy link
Member

What I suggest is modifying our old .pro file to take a command line argument on whether to build tests or not. If the flag is on, it builds the tests instead of main.cpp. This has a lot of duplication between the .pro files.

Also, why is this all of a sudden linking boost?

@jvarley
Copy link
Member Author

jvarley commented Dec 11, 2015

Now has a single .pro file, with CONFIG arg graspit_test. If uncommented, then tests are built. If commented out, then graspit is built.

Also removed boost dependency in my example test, it should not have been there. Added QMAKE_CXXFLAGS += -std=c++0x in order to use std::mutex and std::lock_guard in test rather than boost equivalent.

QMAKE_CXXFLAGS += -std=c++0x
}else{
SOURCES += src/main.cpp
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this back into graspit-core.pro

@mateiciocarlie
Copy link
Member

Almost there - see comments above. main.cpp should be in the core .pro file, and since the tests can only be built under Linux, that part should go into the linux .pro file

…can only be run on linux at the moment. places small graspit_test check in graspit-core.pro to determine if main.cpp should be added to SOURCES.
@jvarley
Copy link
Member Author

jvarley commented Dec 12, 2015

Done.

mateiciocarlie added a commit that referenced this pull request Dec 12, 2015
@mateiciocarlie mateiciocarlie merged commit dc769b3 into graspit-simulator:master Dec 12, 2015
@jvarley jvarley deleted the adding_tests branch January 12, 2016 21:52
jvarley pushed a commit to CRLab/graspit that referenced this pull request Apr 26, 2016
mateiciocarlie pushed a commit that referenced this pull request Jun 29, 2016
Headless working with grasp_planning_graspit
@disdaining disdaining mentioned this pull request Aug 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants