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

Add docker-based ROS prerelease test. #35

Merged
merged 27 commits into from
May 5, 2016

Conversation

130s
Copy link
Member

@130s 130s commented Apr 23, 2016

Adding a feature to run docker-based version of ROS Prerelease Test (let me simply call Prerelease Test for now on Travis CI. This helps the maintainers who are in charge of release into ROS buildfarm.

Motivation

  • Avoid the setup for running Prerelease Test you have to setup your computer (i.e. docker and ros buildfarm packages), which still took 10+ minutes for me today even though this was 3 times I did this.
  • Travis CI provides stably fast computation power (the time it takes for running Prerelease Test locally depends on the machine spec).

Benefit

  • ROS_Buildfarm-quality test at your convenience.
  • You're free from setting up Prerelease Test environment on your machines.
  • Travis CI provides faster computation for the most of developers machines (?need source but that's what's happening to me).
  • Sharing the results of Prerelease Test online!
  • Maintainers of released ROS packages should utilize Prerelease Test more, which can be automatically achieved with it integrated in your Travis config by industrial_ci.

Drawback

  • Not all pull requests are subject to Prerelease Test.
  • Prerelease Test may take longer time to finish.
  • With this PR, Prerelease Test always pass on Travis (I couldn't think of the ways to capture non-zero status code from the current Prerelease Test scripts).
  • Loses configuration flexibility that http://prerelease.ros.org/ provides with its UI. To configure your Prerelease Test with this, you have to modify your Travis config (e.g. .travis.yml).

Usage example
Add this single line to your Travis config (eg. .travis.yml):

ROS_DISTRO=indigo PRERELEASE=true PRERELEASE_REPONAME=industrial_core PRERELEASE_DOWNSTREAM_DEPTH=0

Or in its simplest form:

ROS_DISTRO=indigo PRERELEASE=true

As noted in the updated README, you're advised for now to include the lines like above in allow_failures matrix.

@130s 130s force-pushed the add/dockerbased_prerelease branch from 25705c4 to 02d2d16 Compare April 23, 2016 18:14
@130s
Copy link
Member Author

130s commented Apr 26, 2016

Because this PR is intended primarily to help the maintainers who are in charge of release into ROS buildfarm, I'd like to ask someone who does that for the review. @shaun-edwards and if you can think of anyone could you ask them as well?

I'm now in the process of releasing updates of MoveIt! (I'm helping the release of it) into Jade and would like to include this feature.

@mathias-luedtke
Copy link
Member

Great idea!

Not all pull requests are subject to Prerelease Test.

I could image to setup a release-test branch with the prerelease test only, where one can merge the code in.

I couldn't think of the ways to capture non-zero status code from the current Prerelease Test scripts

I will have a look at it as well :)

@shaun-edwards
Copy link
Member

@ipa-mdl , are you able to review this from ROS release perspective? I'm out on vacation this week and probably won't get to this until next week at the earliest. I don't want to be the hold up on this.

@130s 130s force-pushed the add/dockerbased_prerelease branch from a9472f9 to 88b0104 Compare April 28, 2016 06:00
@130s
Copy link
Member Author

130s commented Apr 28, 2016

Not all pull requests are subject to Prerelease Test.

I could image to setup a release-test branch with the prerelease test only, where one can merge the code in.

Interesting. That will help maintainers who are concerned with the time that the Travis check takes (prerelease test could usually take longer). I've added it in the readme.

I couldn't think of the ways to capture non-zero status code from the current Prerelease Test scripts

I will have a look at it as well :)

I have a feeling that to do what we want we might have to change prerelease*.sh, which could take some time.
I'd rather move forward merging this PR and will work on improvement in the future. How do you think? @ipa-mdl

@@ -64,6 +64,12 @@ function error {
exit 1
}

# Start prerelease, and once it finishs then finish this script too.
if [ "$PRERELEASE" == true ]; then
./ros_pre-release.sh;
Copy link
Member

Choose a reason for hiding this comment

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

This does not work, I would suggest to use the same path composition as for rosdep-install.sh

@mathias-luedtke
Copy link
Member

I have just created a release test branch for ros_canopen (travis: https://travis-ci.org/ros-industrial/ros_canopen/builds/126314370)

Are travis.sh and ros_pre-realease meant to be mutual exclusive?
PRERELEASE=true seems to exit travis.sh directly.
This is not documented (?).

I believe the result of the prerelease test can be checked with catkin_test_results as well.

@130s 130s force-pushed the add/dockerbased_prerelease branch 5 times, most recently from 16a58a8 to 169e863 Compare April 28, 2016 16:24
@130s
Copy link
Member Author

130s commented Apr 30, 2016

@davetcoleman at moveit/moveit_core#285 (comment)

  • Can we move all the if [[ ${PRERELEASE} == true ]]; scripts to a separate bash script in the repo to simplify reading? e.g. .rosPreRelease.sh ?
  • Or could we integrate the pre-release stuff into industrial_ci, where it belongs IMHO?

Yeah, this PR is it.

I'm working to address feedback from Mathias.

@mathias-luedtke
Copy link
Member

I have restarted the travis jobs, they pass now with some issues:

In general:
After everything works the output should be silenced, my jobs reached the 10000 lines display limit (almost 50000 lines in total).
Does travis enforce set -e, if not it should be added to the scripts in order to stop the test on errors.
It really takes a lot of time to pass.. I would suggest to default PRERELEASE_DOWNSTREAM_DEPTH to 0. Or to set no default at all.

@130s 130s force-pushed the add/dockerbased_prerelease branch 4 times, most recently from bb4fbf8 to 1b24397 Compare May 1, 2016 00:05
@130s
Copy link
Member Author

130s commented May 1, 2016

Hope I've addressed all your comments/concern by now.

I couldn't think of the ways to capture non-zero status code from the current Prerelease Test scripts

Now the jobs that run Prerelease Test should fail when PT reports failures.

After everything works the output should be silenced, my jobs reached the 10000 lines display limit (almost 50000 lines in total).

For the output from the current ROS Prerelease Test result, I'm not sure if I can turn it off, nor if we want to. Since the tests are run on the cloud, I'm afraid you won't have an access to the test result files even when error occurs and Prerelease Test advises you to look into those. So I thought having verbose output might be necessary for debugging purpose.

(BTW there are other CI services, e.g. Circle CI, have a feature for you to ssh to the OS the job runs so that in this case, you can take a further look at the test result files.)

}

export CI_SOURCE_PATH=$(pwd)
CI_PARENT_DIR=.ci_config # This is the folder name that is used in downstream repositories in order to point to this repo.
Copy link
Member

Choose a reason for hiding this comment

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

Export is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Fixed now.

@130s
Copy link
Member Author

130s commented May 3, 2016

With the latest commit, all Prerelease Test jobs fail no matter what the PT result is. Example:

https://api.travis-ci.org/jobs/127368871/log.txt?deansi=true

+travis_time_start show_testresult
+set +x
travis_fold:start:show_testresult
travis_time:start:lvwnpik4>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>
+catkin_test_results build
Test results directory "/tmp/prerelease_job/build" does not exist
+error
+travis_time_end 31
+set +x
travis_time:end:lvwnpik4:start=1462232904893485491,finish=1462232904950110626,duration=56625135
travis_fold:end:show_testresult<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
Function show_testresult took 0 min 0 sec
+trap - ERR

I think this is because of the issue in the upstream ros-infrastructure/ros_buildfarm#283, and I'd like to rather move forward for now knowing that PT is not always used and let the upstream issue resolved on its own.

@mathias-luedtke
Copy link
Member

catkin_test_results build

The buildfarm uses isolated builds, so I suggest to use catkin_test_results build_isolated.
If this does not fix the error, we should just not check the results for now.

@130s
Copy link
Member Author

130s commented May 3, 2016

The buildfarm uses isolated builds, so I suggest to use catkin_test_results build_isolated.

I see, I'll give it a try now. But catkin_test_results build is what prerelease.sh prints to suggest when it finishes (can be a bug though).

Let me ask in advance, other than this is this +1 for you? @ipa-mdl

@130s
Copy link
Member Author

130s commented May 3, 2016

Yeah, using catkin_test_results build_isolated also fails probably due to the same issue ros-infrastructure/ros_buildfarm#283 (I feel like tracking down that issue too soon).

+travis_time_start show_testresult
+set +x
�[0Ktravis_fold:start:show_testresult
�[0Ktravis_time:start:lbk0177z�[34m>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>�[0m
+catkin_test_results build_isolated
Test results directory "/tmp/prerelease_job/build_isolated" does not exist
+error
+travis_time_end 31
+set +x
travis_time:end:lbk0177z:start=1462260329553176816,finish=1462260329611520423,duration=58343607�[0K
travis_fold:end:show_testresult�[31m<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<�[0m
�[0K�[31mFunction show_testresult took 0 min 0 sec�[0m
+trap - ERR
+exit 1

@mathias-luedtke
Copy link
Member

Let me ask in advance, other than this is this +1 for you?

+1 if you comment out the catkin_test_results line for now :)

@mathias-luedtke
Copy link
Member

mathias-luedtke commented May 4, 2016

I have tested it locally again:

/tmp/prerelease_job$ catkin_test_results 
Summary: 60 tests, 0 errors, 0 failures

So the trick might be to simply omit the parameter..
Update: --verbose flag should be set for easier debugging.

@130s
Copy link
Member Author

130s commented May 5, 2016

I agree, catkin_test_results --verbose seems to work fine. Fixed.

@130s 130s force-pushed the add/dockerbased_prerelease branch from 0d9cdd3 to 44b0926 Compare May 5, 2016 16:31
@mathias-luedtke
Copy link
Member

https://travis-ci.org/ros-industrial/ros_canopen/builds/126314370 passes now :)
+1 for merge

This PR's checks failed because of some unrelated reasons.

@130s
Copy link
Member Author

130s commented May 5, 2016

@ipa-mdl thanks for all the great reviews!
Let me add you as an author of this feature.

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.

3 participants