ROS Quality Assurance Working Group December 2018 Meeting Notes

Quality Assurance
1

Agenda:

  • Update and discussion of the progress on ongoing initiatives (i.e. QA Dashboard, Code review)
  • It has been a year since this group was established. It is time to reflect upon our work and make some improvements

  1. Quality Dashboard:
    1. Matt Droter is currently running some test to assess the performance of Haros and identify any potential risks on the buildfarm resources.

  2. Code Review:

    1. We selected 3 repositories for a pilot of code review
      1. ros-comm: We raised a PR to add a PR template. The PR has not been merged yet.
      2. rviz: We raised a PR to add a PR template. The PR has not been merged yet.
      3. Moveit: The PR has been merged few weeks ago. However, few PRs have been reviewed as suggested by the checklist, i.e. “While waiting for someone to review your request, please consider reviewing another open pull request to support the maintainers.”
        1. I was asked to investigate why contributors do not review other PRs in the Moveit repository.

  3. Reflection on the last year work:

    1. The process:
      1. Overall, the group is happy with the process. We agreed in some few changes:
        1. Agree on a fixed day on the month to meet, instead of sending the Doodle vote every month. Hence, we will vote on which day of the month we will meet. This is the Doodle link https://doodle.com/poll/q6isx4ar9yd4awxw
        2. The group seem to be happy with me facilitating the meetings. In case I’m not available, I’ll nominate someone to facilitate the meeting.
    2. The progress:
      1. The group acknowledges that the progress is modest. This due to mainly two constraints: quality is a “difficult subject” in the ROS community and there is lack of contributors to our initiatives. We struggle to find ROS contributors to help.

  4. Actions:

    1. Investigate why moveit contributors do not review other PRs while waiting for their PR to be reviewed (Adam).
    2. Send out a Doodle link to vote on which day of the month to meet (Adam).
2 Likes
2

My personal thoughts with regards to MoveIt contributors don’t review other PRs:
It took me a long time to feel like I understood MoveIt enough to contribute, and it definitely takes longer to be able to understand someone else’s contribution, especially if it’s in a part of the code that you aren’t familiar with. MoveIt’s a very big project, so this happens pretty frequently: personally, I don’t feel like I have enough knowledge to review anything outside of MoveIt Core and the OMPL interface, and don’t do so often.

However, I think the biggest problem is that it’s not clear how to review a PR when you aren’t a maintainer. While some comments in reviews are algorithmic or performance based, I think you could say that 30-50% of a code review is subjective, given that there are a lot of equally valid ways to do the same thing. Personally, I’d rather not review a PR with some advice, only for that advice to contradict the advice of the maintainers (or anyone else with more experience than me) when they review it and end up confusing first time contributors.

In general, It’s not clear how to help the maintainers the best when reviewing. Maybe there should be a section in CONTRIBUTING about reviewing other PRs, what to focus on, what to leave to the maintainers for codebase consistency, etc.

3 Likes
3

These are good points you raised. And inline with what other contributors conveyed to me.
Regarding documentation, we put together a mini code review guide. Please, have a look? Any feedback is appreciated.