rework of moveit_ci testing framework for Travis #45

rhaschke · 2019-01-28T00:56:34Z

Being originally started in #44 from fixing several basic issues in moveit_ci scripts to make #42 and #43 work, I ended up with a thorough rework of the basic moveit_ci scripts in util.sh based on most recent scripts at travis-build.

Issues resolved:

BEFORE_SCRIPTS didn't run correctly for a while. For example here testing should be printed, but it's not. I fixed this, by evaluating commands passed to travis_run_*.
Getting clang-tidy running wasn't trivial as well, e.g. putting .clang-tidy into the correct location and convincing Travis to not bail out.
Building and running tests wasn't executed with travis_run_wait, i.e. timeout and . generation.

Improvements:

The key improvements are in f3ac41d
- better modularization
  - travis_run_simple: no folding
  - travis_run_true: folding, timing, accept failures
  - travis_run: folding, timing, not accepting failures
  - travis_run_wait: +timeout
- support hierarchical folds (with custom fold names and titles)
- travis_wait() to wait for a background process to finish
- limit travis_run_wait's timeout to estimated remaining Travis' build time
  When we hit Travis' hard timeout, caches are not saved by Travis anymore. Hence we need to bail out before Travis to get our caches saved (and thus have a chance on the next build to meet the timeout deadline). To this end, I estimate the remaining build job time and compare against the configurable variable MOVEIT_CI_TRAVIS_TIMEOUT, which defaults to 45min. Although by default open-source projects at http://travis-ci.org have a timeout of 50 mins, a have put some safety-margin in, because booting Travis' virtual machine, loading caches, etc. already consumes time that I cannot measure.
- filter() and filter-out() utility functions

This whole PR is coming with a whole new bash-based unittest suite to validate correctness of all bash functions.

All smaller commits contain independent and self-explanatory changes, which is why this PR should be merge-committed as discussed in moveit/moveit_tutorials#280 (comment) ;-)

Commands were not actually executed.

davetcoleman

This is a really impressive contribution!
Of course its a bit large for a single PR and I did make some requests, but overall thank you @rhaschke !

.clang-format

.travis.yml

check_clang_tidy.sh

check_warnings.sh

test_pkgs/catkin_lint/test.cpp

util.sh

davetcoleman · 2019-01-28T06:42:37Z

util.sh

 #********************************************************************
 # Software License Agreement (BSD License)
 #
-#  Copyright (c) 2016, University of Colorado, Boulder
+#  Copyright (c) 2018, Bielefeld University


this is odd to me you're changing the copyright but i guess you've changed this file enough for it to be a different file. i wouldn't make this a habit but i don't really care here.

Yes, this file is more or less new. The copyright is not on the file (name), but it's content, right? 😉
Note, that you're still mentioned as a co-author as you developed some original ideas for it.
By the way, you originally copied most of those functions from travis-build without mentioning it. 😉

Thanks for bringing up copyright stuff to my attention. I added the original MIT license from travis-build to the file travis_functions.sh.

rhaschke · 2019-01-28T10:39:26Z

Most of the clang-tidy warnings are outside of moveit_core, which so far is the only part of the codebase that's been tidied, so that's expected. There are a few warnings in moveit_core that I don't remember seeing, such as this and this.

2nd this is due to code moved from moveit_experimental to moveit_core, which is for this reason not yet tidied.
1st this is new code too.

The command "catkin build --no-status --summarize" reached the internal timeout of 45 minutes. Aborting.

Even with Travis's help, catkin isn't cooperating. Maybe running catkin build and clang-tidy separately again would be better?

I have introduced an own, internal timeout to ensure that Travis can write the cache.
For this internal timeout, I didn't yet adjusted the default.

Any reason you didn't copy over the identifier naming rules from the MoveIt repo?

This is a left-over from debugging. Will remove it.

rhaschke · 2019-01-28T21:36:14Z

Closing again for more debugging...

davetcoleman · 2019-01-28T21:48:43Z

I'd prefer you not keep closing PRs and re-opening them, its confusing. The "WIP" is enough signaling, or just leave a comment.

I believe @RoboticsYY is interested in helping out with the clang-tidy effort, and this is related.

rhaschke · 2019-01-28T22:23:56Z

I'd prefer you not keep closing PRs and re-opening them, its confusing.

Sorry. I want to avoid double load on Travis (building both, the repository branch and the PR).
I will send a notice when I'm ready for a new review round.

RoboticsYY · 2019-01-29T02:47:57Z

I'm making some efforts to the clang-tidy issue according to the roadmap proposed by @BryceStevenWilley in #28. Looks like @rhaschke has already put clang-tidy to the travis checks. My current plan is to apply clang-tidy fix to the entire codebase.

rhaschke · 2019-01-29T06:37:20Z

@RoboticsYY Thanks for your efforts. Hopefully your PR will not conflict too much with existing PRs for MoveIt. Please split your PR into individual commits for each tidy step, much like @BryceStevenWilley did for moveit_core (moveit/moveit#880). Particularly, also split up automatic fixups and manual corrections to them. This will facilitate the review process.

RoboticsYY · 2019-01-29T10:34:21Z

@rhaschke Thanks for reminding me of that. I have noticed @BryceStevenWilley 's way of splitting the commits. I also think it's a good practice to follow for the clang-tidy PR.

- more modularization - travis_run_simple: no folding - travis_run_true: folding, timing, accept failures - travis_run: folding, timing, not accepting failures - travis_run_wait: +timeout - travis_wait() to wait for a background process to finish - support hierarchical folds (with custom fold names and titles) and use them to cleanup the Travis ouput - limit travis_run_wait's timeout to estimated remaining Travis' build time When we hit Travis' hard timeout, caches are not saved by Travis anymore. Hence we need to bail out before Travis to get our caches saved (and thus have a chance on the next build to meet the timeout deadline). To this end, I estimate the remaining build job time and compare against the configurable variable MOVEIT_CI_TRAVIS_TIMEOUT, which defaults to 45min. Although by default open-source projects at http://travis-ci.org have a timeout of 50 mins, a have put some safety-margin in, because booting Travis' virtual machine, loading caches, etc. already consumes time that I cannot measure. - filter() and filter-out() utility functions - extensive unit tests for all these utility functions - renamed misleading variable CI_PARENT_DIR to MOVEIT_CI_DIR

This allows to perform clang-* checks (which only operate on the to-be-tested repo) during Travis testing of the moveit_ci repository (which itself doesn't have source code)

rhaschke · 2019-01-30T12:46:15Z

dropping the clang-analyzer-* checks seem reasonable ... as long as they stay enabled in the main repo

This is hardly possible. The philosophy of moveit_ci is:
If repositories bring their own .clang-format or .clang-tidy config file, they should be used.
The key idea of using clang-tidy, both manually by the user and on Travis, is to apply applicable fixes and thus ensure high code quality automatically, without the need for manual reviewing (as far as possible). For clang-format this works quite well as you pointed out.
I think clang-tidy can tremendously help us on naming conventions in the same fashion. However, the code analysis checks don't automatically apply fixes (at least most of them do not). They are "only" useful for revealing logical program errors. I think in a later step they will be helpful too.
For now, they still report too many errors, probably also some false-positives. This wouldn't be helpful for automatic checking on Travis.

One option to achieve the distinction between fixing and checking, is to provide two different files: .clang-tidy and .clang-tidy-fix. The former would include full code analysis, while the latter would only include auto-fixable checks. However, to activate one or the other, Travis would need to modify the source tree, which will cause issues with other checks, e.g. clang-format, which just look at changes in the source repo.

So far, we had to blacklist meta-packages as well, because they pulled in real packages.

run basic integration tests with integration_tests.sh script

rhaschke · 2019-01-30T21:44:25Z

@davetcoleman I fixed some further smaller issues. Can we merge this and then enable, the new warnings checker for MoveIt, now that MoveIt compiles without warnings?
The catkin and clang-tidy linters can only be enabled if the current issues are resolved (moveit/moveit#1137 for Melodic).
Note that the two Travis tests fail, because the MoveIt has catkin_lint warnings and is not yet tidied.

Co-Authored-By: rhaschke <rhaschke@users.noreply.github.com>

davetcoleman

Please just fix the licenses

davetcoleman · 2019-01-30T17:55:14Z

README.md

- Clean Travis log files - looks similiar to a regular .travis.yml file
- Runs tests for the current repo, e.g. if testing moveit\_core only runs tests for moveit\_core
+- Uses [pre-build Docker containers](https://hub.docker.com/r/moveit/moveit) for all ROS distros to save setup time
+- Simple Travis configuration


Yes thank you for these efforts, I'm trying to do reviews quickly because I'm excited about these improvements!

check_catkin_lint.sh

check_clang_format.sh

check_clang_tidy.sh

check_warnings.sh

davetcoleman · 2019-01-30T17:58:36Z

function_tests.sh

+#     with the distribution.
+#   * Neither the name of Bielefeld University nor the names of its
+#     contributors may be used to endorse or promote products derived
+#     from this software without specific prior written permission.


Here its ok not to say 3-clause because the full license makes it obvious its the 3-clause

davetcoleman

Just noticed you already fixed the license stuff

- sanity check for empty workspace - some cleanup

rhaschke added 2 commits January 24, 2019 20:02

fix running BEFORE_SCRIPTS

7460a18

Commands were not actually executed.

get rid of "Terminated travis_jigger $cmd_pid $timeout $cmd" messages

586bee5

davetcoleman requested changes Jan 28, 2019

View reviewed changes

rhaschke changed the title ~~rework of moveit_ci testing framework for Travis~~ WIP: rework of moveit_ci testing framework for Travis Jan 28, 2019

rhaschke force-pushed the clang-tidy-check branch from 8e4c022 to ed0e12a Compare January 28, 2019 21:15

rhaschke closed this Jan 28, 2019

rhaschke reopened this Jan 29, 2019

rhaschke force-pushed the clang-tidy-check branch from 76231e9 to 8e56cfe Compare January 29, 2019 06:20

BryceStevenWilley mentioned this pull request Jan 29, 2019

Apply clang-tidy code linter moveit/moveit#28

Closed

rhaschke force-pushed the clang-tidy-check branch from 8e56cfe to c803c30 Compare January 29, 2019 10:04

rhaschke force-pushed the clang-tidy-check branch from 14c4d50 to 2ec63ab Compare January 29, 2019 12:22

rhaschke added 2 commits January 29, 2019 15:05

accept CI_SOURCE_PATH from Travis

6a32678

This allows to perform clang-* checks (which only operate on the to-be-tested repo) during Travis testing of the moveit_ci repository (which itself doesn't have source code)

rhaschke force-pushed the clang-tidy-check branch 5 times, most recently from 83e339d to 1b0d9b4 Compare January 29, 2019 15:44

rhaschke added 3 commits January 29, 2019 21:43

fix clang-tidy

0aaa15c

unittest packages

d1c2816

provide a well-defined clang-tidy config for unit tests

97c4c26

rhaschke force-pushed the clang-tidy-check branch 2 times, most recently from be044bc to d2064c0 Compare January 29, 2019 21:12

rhaschke added 6 commits January 30, 2019 14:31

check for warnings

97d9c24

catkin_lint check

ab0bb35

simplify TEST_BLACKLIST handling

959087a

So far, we had to blacklist meta-packages as well, because they pulled in real packages.

restructured tests

b87054a

run basic integration tests with integration_tests.sh script

improve cloning from github into the workspace: allow branch specifier

21f7144

fancy progress indicator

b5f36ae

rhaschke force-pushed the clang-tidy-check branch from 5e73209 to 5fe6dac Compare January 30, 2019 13:31

combined TEST=catkin_lint,clang-format

f8b6f75

rhaschke force-pushed the clang-tidy-check branch 3 times, most recently from 0a78901 to 87bb496 Compare January 30, 2019 20:09

run-clang-tidy: also print detailed change info (in separate fold)

6bda9f1

rhaschke force-pushed the clang-tidy-check branch from 87bb496 to 862e813 Compare January 30, 2019 21:35

rhaschke force-pushed the clang-tidy-check branch from 862e813 to 356a43d Compare January 30, 2019 22:35

specify more concrete BSD-3 clause

090bec9

Co-Authored-By: rhaschke <rhaschke@users.noreply.github.com>

rhaschke force-pushed the clang-tidy-check branch 2 times, most recently from 63dc379 to a48db4e Compare January 30, 2019 23:50

davetcoleman requested changes Jan 30, 2019

View reviewed changes

davetcoleman approved these changes Jan 30, 2019

View reviewed changes

structure travis.sh into several functions

7d452d0

- sanity check for empty workspace - some cleanup

rhaschke force-pushed the clang-tidy-check branch from 04caa36 to 7d452d0 Compare January 31, 2019 13:42

rhaschke merged commit 5234e97 into master Jan 31, 2019

rhaschke deleted the clang-tidy-check branch January 31, 2019 14:43

This was referenced Feb 1, 2019

Travis: enable warnings and catkin_lint checker moveit/moveit#1332

Merged

Add clang-tidy #12

Closed

RoboticsYY mentioned this pull request Feb 18, 2019

Clang tidy moveit/moveit#1354

Closed

1 task

rhaschke mentioned this pull request Mar 9, 2019

Add catkin_lint #11

Closed

rhaschke mentioned this pull request May 2, 2020

Debians and circular dependency with moveit_ros_occupancy_map_monitor and moveit_ros_perception moveit/moveit#2042

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rework of moveit_ci testing framework for Travis #45

rework of moveit_ci testing framework for Travis #45

rhaschke commented Jan 28, 2019

davetcoleman left a comment

davetcoleman Jan 28, 2019

rhaschke Jan 28, 2019 •

edited

Loading

rhaschke commented Jan 28, 2019 •

edited

Loading

rhaschke commented Jan 28, 2019

davetcoleman commented Jan 28, 2019

rhaschke commented Jan 28, 2019 •

edited

Loading

RoboticsYY commented Jan 29, 2019

rhaschke commented Jan 29, 2019

RoboticsYY commented Jan 29, 2019

rhaschke commented Jan 30, 2019

rhaschke commented Jan 30, 2019

davetcoleman left a comment

davetcoleman Jan 30, 2019

davetcoleman Jan 30, 2019

davetcoleman left a comment

rework of moveit_ci testing framework for Travis #45

rework of moveit_ci testing framework for Travis #45

Conversation

rhaschke commented Jan 28, 2019

davetcoleman left a comment

Choose a reason for hiding this comment

davetcoleman Jan 28, 2019

Choose a reason for hiding this comment

rhaschke Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

rhaschke commented Jan 28, 2019 • edited Loading

rhaschke commented Jan 28, 2019

davetcoleman commented Jan 28, 2019

rhaschke commented Jan 28, 2019 • edited Loading

RoboticsYY commented Jan 29, 2019

rhaschke commented Jan 29, 2019

RoboticsYY commented Jan 29, 2019

rhaschke commented Jan 30, 2019

rhaschke commented Jan 30, 2019

davetcoleman left a comment

Choose a reason for hiding this comment

davetcoleman Jan 30, 2019

Choose a reason for hiding this comment

davetcoleman Jan 30, 2019

Choose a reason for hiding this comment

davetcoleman left a comment

Choose a reason for hiding this comment

rhaschke Jan 28, 2019 •

edited

Loading

rhaschke commented Jan 28, 2019 •

edited

Loading

rhaschke commented Jan 28, 2019 •

edited

Loading