Unit tests CMake and Travis refactoring #1786

SergioRAgostinho · 2016-12-25T00:49:48Z

Hey everyone, here's my Christmas 🎁 :) This PR addresses the following:

The unit tests CMake files were changed to allow: running the test for a certain module, as long as the module is build; specify if the tests for a certain module should be built.
All dependencies in travis are now installed directly from aptitude.
The full build now also builds tools, examples, apps and the simulation module which was previously not being built I think.
I split the unit tests on travis into two jobs. I packed things together in a way that ensures that all dependencies are met (even the new optional ones), so that all tests run. The new "test-extended" job is running on a somewhat tight schedule, so it might need some off-balancing somewhere in the near future, but this should be enough for now.

Modules test dependencies (optional in parenthesis):

common: (io features)
geometry: common
ml: common
octree: common
io: octree (visualization)
kdtree: common (io)
search: kdtree octree (io)
visualization: io search (features)
sample_consensus: search (io)
filters: sample_consensus (io features segmentation)
outofcore: io filters visualization
2d: io filters
features: filters 2d (io keypoints)
registration: features (io)
keypoints: features (io)
surface: search (io features filters)
segmentation: geometry features ml
people: visualization segmentation
recognition: io registration ml

Test Matrix

Core	Extended
common: (io features)	filters: sample_consensus (io features segmentation)
geometry: common	registration: features (io)
octree: common	surface: search (io features filters)
kdtree: common (io)	segmentation: geometry features ml
io: octree (vtk)	people: visualization segmentation
search: kdtree octree (io)	recognition: io registration ml
sample_consensus: search (io)
visualization: io search geomtry (features)
2d: io filters
outofcore: io filters visualization
keypoints: features (io)
features: filters 2d (io keypoints)

Updated

Core	Extended 1	Extended 2
common: (io features)	io: octree (vtk)	filters: sample_consensus (io features segmentation)
geometry: common	visualization: io search geomtry (features)	segmentation: geometry features ml
octree: common	outofcore: io filters visualization	people: visualization segmentation
kdtree: common (io)	registration: features (io)	recognition: io registration ml
search: kdtree octree (io)	surface: search (io features filters)
sample_consensus: search (io)
2d: io filters
keypoints: features (io)
features: filters 2d (io keypoints)

Closes #1785.

Merry Christmas 🎅

taketwo · 2016-12-25T09:04:11Z

Hi Sergio, great work! I'll have a detailed look once I'm back in the office next week. Merry Christmas!

maflcko · 2016-12-31T01:07:39Z

.travis.yml

@@ -33,7 +33,8 @@ env:
 matrix:
  include:
    - compiler: gcc
-      env: TASK="test"
+      env: TASK="test-core"
+    - env: TASK="test-extended"


You may need to set the compiler for this task* as well. Otherwise you might end up with ["gcc", "clang"] set as compiler, I think?

* same for doc

maflcko · 2017-04-04T08:01:06Z

fyi, I've been running the segmentation module without issues for a couple of months now.

SergioRAgostinho · 2017-04-18T16:08:34Z

Hey guys :) Can I get a volunteer to have a look at these? I have some spare time the next two weeks and wanted to have the tests running before starting to review PRs.

taketwo

I'd like to see that last commit of Marco running on Travis, but somehow it does not pick it up and still runs the broken version: https://travis-ci.org/PointCloudLibrary/pcl/jobs/186595213

taketwo · 2017-04-18T17:02:11Z

test/keypoints/CMakeLists.txt

-                 ARGUMENTS "${PCL_SOURCE_DIR}/test/cturtle.pcd")
+set(SUBSYS_NAME tests_keypoints)
+set(SUBSYS_DESC "Point cloud library keypoints module unit tests")
+set(SUBSYS_DEPS global_tests keypoints common search kdtree octree features filters)


How do you decide which modules are SUBSYS_DEPS for a particular test subsystem?

I mimic the dependencies from the library/module they belong i.e.: test_keypoints has all the deps the module keypoints has. I don't remember exactly why I added all the recursive dependencies explicitly, but I think it's to ensure the include directories and link flags are properly defined when creating each individual test target.

It's been a while now, my memory is not that fresh :/

Sorry for this delay in reviewing!
So basically these are propagated from the respective module under test. Do you recall any reasons this can not be done automatically? Would definitely improve maintainability.

Sorry for the massive delay 😅 . I found a way to mimic the behaviour you described, to I'm applying the changes now. The doxygen issue after...

taketwo · 2017-04-18T17:18:43Z

.travis.yml

+      - libqhull-dev
+      - libvtk5-dev
+      - libflann-dev
+      - doxygen


This brings us an ancient version of Doxygen from 2011 (1.7.6 to be precise). Apart from gazillion bug fixes, one useful feature that is missing there is Markdown support. Back in the day I even made a PR (#526) to get a newer version. I'm not sure how much of PCL docstrings are actually written with Markdown, but the fact that I made that request suggests that there are at least some.

This looks exactly like the solution we need. Doxygen packages backported for Precise. It will bump the version to 1.8.8 which already has markdown support.

SergioRAgostinho · 2017-04-18T18:43:50Z

I'd like to see that last commit of Marco running on Travis, but somehow it does not pick it up and still runs the broken version: https://travis-ci.org/PointCloudLibrary/pcl/jobs/186595213

You're right. I'll have a look later today.

SergioRAgostinho · 2017-05-17T12:04:55Z

I'd like to see that last commit of Marco running on Travis, but somehow it does not pick it up and still runs the broken version: https://travis-ci.org/PointCloudLibrary/pcl/jobs/186595213

So there's an issue with that given this particular line, since this is in fact a pull request

if [[ $TRAVIS_PULL_REQUEST != 'false' ]]; then exit; fi

But there's still something off, even with the CI job on my local fork. Is there any reason for us to clone the doc repo like this

git clone git@github.com:PointCloudLibrary/documentation.git .

instead of through the usual url? That one ended up failing in my local CI job because it requested me to enter an RSA password (which I assume is somehow encoded in all those 'secure' lines in the travis.yml).

taketwo · 2017-05-17T12:13:02Z

We clone via SSH to later allow password-less push of the newly generated docs.
Perhaps for testing we can move this line in question further down, so that only final push is skipped? And before merging this PR move it back.

SergioRAgostinho · 2017-05-17T12:16:19Z

Let me give it a try then

SergioRAgostinho · 2017-05-17T13:15:27Z

It's suffering from the same issue as my local job (which is expected). A password is prompted to access the RSA file which results in the job hanging and eventually failed due to lack of output for 10 minutes.

Any ideas on how to overcome this?

maflcko · 2017-05-17T13:28:58Z

You can replace the git clone by ``` git clone https://github.com/PointCloudLibrary/pcl.git . # Don't auth with gh for now ``` then remove the lines that contain "if [[ $TRAVIS_PULL_REQUEST != 'false' ]]; then exit; fi" and "git push --force"

…

On Wed, May 17, 2017 at 3:15 PM, Sérgio Agostinho ***@***.***> wrote: It's suffering from the same issue <https://travis-ci.org/PointCloudLibrary/pcl/jobs/233212848#L1255> as my local job (which is expected). A password is prompted to access the RSA file which results in the job hanging and eventually failed due to lack of output for 10 minutes. Any ideas on how to overcome this? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1786 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmv5j-bRMjs0IncwRKonBXoKSPXyoNks5r6vL2gaJpZM4LVUe9> .

taketwo · 2017-05-17T13:35:47Z

Marco probably meant https://github.com/PointCloudLibrary/documentation.git

SergioRAgostinho · 2017-05-17T13:45:51Z

I did that before, here's the job output on that one (it's from my forks CI but that's pretty much equivalent)
https://travis-ci.org/SergioRAgostinho/pcl/jobs/233205797
It builds, it simply can't push (as expected).

taketwo · 2017-05-17T13:49:07Z

Ok, then I think this part is fine.
But as far as I can see, in the meanwhile the extended test job started to consistently time out.

SergioRAgostinho · 2017-05-17T13:59:25Z

But as far as I can see, in the meanwhile the extended test job started to consistently time out.

'test-extended' is the one which is failing most consistently from time out
'test-core' has an actual test failing and on this last one it took 13 minutes more than usual, timing out.

These timing fluctuations are cumbersome. I guess the underlying point you're trying to make is, to reconsider splitting the current tasks even more.

taketwo · 2017-05-17T14:04:59Z

Well yes, that is the only thing that prevents us from merging.

SergioRAgostinho · 2017-05-17T15:15:26Z

Updated the table and test batches. Tried to run again make with -j2. It might run out of memory but let's see.

SergioRAgostinho · 2017-05-17T17:54:01Z

It should be stable for a while now. I opted to partition the building of apps, examples and tools into their own individual jobs, to ensure the best probabilities of not running out of time.

Now it's a matter of deciding which commits to squash (especially these last ones).

taketwo · 2017-05-18T10:17:43Z

Seems to work now! Indeed, makes sense to squash some of the latest commits.

Regarding the failing test case: it tests the behavior of concatenatePointClouds in presence of "padding" fields. Unfortunately, the expected behavior is not documented, so we can only guess from the code how it is supposed to work. After a quick look-through, I think the rgb field should remain unititialized, so it's not clear why it is being tested for equality to zero and why this succeeded before. Well anyway, it's a separate issue.

SergioRAgostinho · 2017-05-18T15:24:56Z

Seems to work now! Indeed, makes sense to squash some of the latest commits.

So after the review process is complete (not sure if you're done with it), I would propose to squash everything into two main commits: one pertaining the test refactoring (CMake and source files) and the other one related to the changes done in Travis script and configuration files.

@MarcoFalke If I squash your commit it will omit your contribution. Would this be a problem? I can always try to keep it separate during the rebase process.

maflcko · 2017-05-18T15:34:02Z

Feel free to squash. The contribution wasn't substantial anyway.

…

On Thu, May 18, 2017 at 5:25 PM, Sérgio Agostinho ***@***.***> wrote: Seems to work now! Indeed, makes sense to squash some of the latest commits. So after the review process is complete (not sure if you're done with it), I would propose to squash everything into two main commits: one pertaining the test refactoring (CMake and source files) and the other one related to the changes done in Travis script and configuration files. @MarcoFalke <https://github.com/marcofalke> If I squash your commit it will omit your contribution. Would this be a problem? I can always try to keep it separate during the rebase process. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1786 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmv4JoEPAEbsshZX04vXHPBNQKkmR4ks5r7GLNgaJpZM4LVUe9> .

- Include directories are now added for opt dependencies - Make module dependencies propagate to the corresponding tests

@MarcoFalke

- install dependencies from package manager - Full build now also builds simulation, apps and examples - tests refactoring, now split into core and extended - Specify compiler explicitly on all jobs, reset author and date for docs (author @MarcoFalke) - Add ppa of backported doxygen for Precise. Allows having a more recent version than the default one on Precise which already supports markdow. - Prevent doc push in case of a pull request - Refactor into three test blocks - Limit the number of jobs in the extension tests - Refactored compilation of lib, apps, tools and examples

SergioRAgostinho · 2017-05-19T13:58:55Z

Et voilá. Let's allow it to run and then finally conclude this guy 😄

taketwo · 2017-05-19T14:00:17Z

@jspricke fine to merge?

jspricke · 2017-05-30T05:52:07Z

Thanks a lot guys and sorry for keep you waiting :).

SergioRAgostinho added the needs: code review Specify why not closed/merged yet label Dec 25, 2016

SergioRAgostinho added this to the pcl-1.8.1 milestone Dec 25, 2016

maflcko reviewed Dec 31, 2016

View reviewed changes

maflcko mentioned this pull request Jan 27, 2017

libpcl_geometry did not build #1810

Closed

taketwo reviewed Apr 18, 2017

View reviewed changes

SergioRAgostinho force-pushed the tests_refactor branch from ab34ef6 to 7624b8b Compare May 17, 2017 12:19

Modularized unit tests

2c5c1cb

- Include directories are now added for opt dependencies - Make module dependencies propagate to the corresponding tests

taketwo approved these changes May 19, 2017

View reviewed changes

SergioRAgostinho force-pushed the tests_refactor branch from c72e2b8 to 372fae5 Compare May 19, 2017 13:57

taketwo mentioned this pull request May 22, 2017

Adds in-memory PCD serialization/deserialization; de-duplicates PCDReader::readHeader(). #1869

Closed

jspricke merged commit 2446343 into PointCloudLibrary:master May 30, 2017

taketwo removed the needs: code review Specify why not closed/merged yet label May 30, 2017

SergioRAgostinho deleted the tests_refactor branch May 30, 2017 09:05

SergioRAgostinho mentioned this pull request May 18, 2017

pcl_io test failing #1866

Closed

SergioRAgostinho mentioned this pull request Oct 8, 2017

[TRAVIS] Disable VTK in every test. #2018

Closed

SergioRAgostinho mentioned this pull request Nov 26, 2017

Modification of the Travis tasks #699

Closed

SergioRAgostinho mentioned this pull request Jan 11, 2018

Split (yet again) Travis test job into two and tweak timings in building apps #2182

Merged

SunBlack mentioned this pull request Aug 14, 2022

[cmake] Combine PCL_SUBSUBSYS_DEPEND with PCL_SUBSYS_DEPEND #5379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit tests CMake and Travis refactoring #1786

Unit tests CMake and Travis refactoring #1786

SergioRAgostinho commented Dec 25, 2016 •

edited

Loading

taketwo commented Dec 25, 2016

maflcko Dec 31, 2016 •

edited

Loading

maflcko commented Apr 4, 2017

SergioRAgostinho commented Apr 18, 2017

taketwo left a comment

taketwo Apr 18, 2017

SergioRAgostinho Apr 18, 2017 •

edited

Loading

taketwo Apr 18, 2017

SergioRAgostinho May 16, 2017

taketwo Apr 18, 2017

SergioRAgostinho May 17, 2017

SergioRAgostinho commented Apr 18, 2017 •

edited

Loading

SergioRAgostinho commented May 17, 2017

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017

SergioRAgostinho commented May 17, 2017

maflcko commented May 17, 2017 via email

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017 •

edited

Loading

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017

SergioRAgostinho commented May 17, 2017 •

edited

Loading

taketwo commented May 18, 2017

SergioRAgostinho commented May 18, 2017

maflcko commented May 18, 2017 via email

SergioRAgostinho commented May 19, 2017

taketwo commented May 19, 2017

jspricke commented May 30, 2017

Unit tests CMake and Travis refactoring #1786

Unit tests CMake and Travis refactoring #1786

Conversation

SergioRAgostinho commented Dec 25, 2016 • edited Loading

taketwo commented Dec 25, 2016

maflcko Dec 31, 2016 • edited Loading

Choose a reason for hiding this comment

maflcko commented Apr 4, 2017

SergioRAgostinho commented Apr 18, 2017

taketwo left a comment

Choose a reason for hiding this comment

taketwo Apr 18, 2017

Choose a reason for hiding this comment

SergioRAgostinho Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

taketwo Apr 18, 2017

Choose a reason for hiding this comment

SergioRAgostinho May 16, 2017

Choose a reason for hiding this comment

taketwo Apr 18, 2017

Choose a reason for hiding this comment

SergioRAgostinho May 17, 2017

Choose a reason for hiding this comment

SergioRAgostinho commented Apr 18, 2017 • edited Loading

SergioRAgostinho commented May 17, 2017

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017

SergioRAgostinho commented May 17, 2017

maflcko commented May 17, 2017 via email

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017 • edited Loading

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017

taketwo commented May 17, 2017

SergioRAgostinho commented May 17, 2017

SergioRAgostinho commented May 17, 2017 • edited Loading

taketwo commented May 18, 2017

SergioRAgostinho commented May 18, 2017

maflcko commented May 18, 2017 via email

SergioRAgostinho commented May 19, 2017

taketwo commented May 19, 2017

jspricke commented May 30, 2017

SergioRAgostinho commented Dec 25, 2016 •

edited

Loading

maflcko Dec 31, 2016 •

edited

Loading

SergioRAgostinho Apr 18, 2017 •

edited

Loading

SergioRAgostinho commented Apr 18, 2017 •

edited

Loading

SergioRAgostinho commented May 17, 2017 •

edited

Loading

SergioRAgostinho commented May 17, 2017 •

edited

Loading