Skip to content
This repository was archived by the owner on May 31, 2025. It is now read-only.

Add missing inter-test dependency#788

Merged
gerkey merged 1 commit intokinetic-develfrom
fix_test_rosbag_deps
Apr 12, 2016
Merged

Add missing inter-test dependency#788
gerkey merged 1 commit intokinetic-develfrom
fix_test_rosbag_deps

Conversation

@gerkey
Copy link
Copy Markdown
Contributor

@gerkey gerkey commented Apr 11, 2016

Fixes #787.

I'm not sure what caused this problem to suddenly pop up, but I would guess that it's a side effect of using a newer version of cmake/ctest that no longer runs tests in the order that they're declared in the CMakeLists.txt. In any case we never should have assumed that that was happening.

A more thorough cleanup step would be to restructure these three tests to not depend on each others' side effects in the file system. But adding the missing dependency in this manner will at least restore the previous behavior.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 11, 2016

Seems like a reasonable to workaround to me. However, it would probably be better if the bag generation was done in each test if it doesn't already exist. For example, I imagine it makes it impossible to just run the test that was failing before (without first running the test that generates the bag).

@gerkey
Copy link
Copy Markdown
Contributor Author

gerkey commented Apr 11, 2016

Indeed, with this change, if you make run_tests_test_rosbag_rostest_test_random_play.xml, you'll first get a(nother) run of the random_record test. That seems to me a minor inconvenience and a practical trade-off compared to restructuring the tests.

@wjwwood
Copy link
Copy Markdown
Member

wjwwood commented Apr 11, 2016

Sure, I'm in favor of merging as-is.

@gerkey
Copy link
Copy Markdown
Contributor Author

gerkey commented Apr 11, 2016

First try didn't work due to more-complex-than-expected target structure around the tests. f705e7f will hopefully do it, using the DEPENDENCIES keyword arg to add_rostest()

@gerkey
Copy link
Copy Markdown
Contributor Author

gerkey commented Apr 12, 2016

Hooray! http://build.ros.org/job/Kpr__ros_comm__ubuntu_xenial_amd64/49/

@gerkey gerkey force-pushed the fix_test_rosbag_deps branch from f705e7f to be512aa Compare April 12, 2016 00:21
@gerkey gerkey merged commit 536f0c7 into kinetic-devel Apr 12, 2016
@gerkey gerkey deleted the fix_test_rosbag_deps branch April 12, 2016 00:22
@dirk-thomas
Copy link
Copy Markdown
Member

dirk-thomas commented Apr 12, 2016

Cherry-picked to indigo-devel: 535f160

rsinnet pushed a commit to MisoRobotics/ros_comm that referenced this pull request Jun 19, 2017
@gerkey
Merge pull request ros#788 from ros/fix_test_rosbag_deps
1a92e68 
Add missing inter-test dependency
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gerkey @wjwwood @dirk-thomas