ros / rosdistro

This repo maintains a lists of repositories for each ROS distribution
Other
931 stars 2.56k forks source link

Repo check test is sometimes checking or reporting errors in the wrong lines #31439

Open ivanpauno opened 2 years ago

ivanpauno commented 2 years ago

I have seen two differece instances of the issue:

screenshot ![image](https://user-images.githubusercontent.com/26796393/145611638-5d820301-db97-469c-b068-5db3117c2be4.png) ![image](https://user-images.githubusercontent.com/26796393/145611680-0cb5bc30-1122-4306-88fd-500368020de7.png)
screenshot ![image](https://user-images.githubusercontent.com/26796393/145612019-d279e538-fc1b-46a1-a1cc-aa4c6bf98f7d.png) ![image](https://user-images.githubusercontent.com/26796393/145612058-426041a1-99e3-4c30-9d8c-eb43976b7eb2.png)

Both things are likely different issues though.

The first issue is maybe related to my comment here. The second one is likely related to the diff having some extra context lines. (those are just wild guesses)

ivanpauno commented 2 years ago

cc @cottsay

cottsay commented 2 years ago

Alright, I've reproduced two issues that the current implementation is facing.

  1. The HEAD in the checked out repository can be a merge commit with master, which is why the line numbers can get off.
  2. If master has a commit which removes rules from rosdep but the PR branch does not, the diff shows those lines as being new.

Based on this, I think we need to: a. Introduce a third ref in the workflow to represent the PR's tip. The local checkout should continue to use the merge so that the testing infrastructure is up-to-date. So the three important refs are the merge, the PR tip, and the target tip. b. Use git's triple-dot to indicate that we only want to see "new" commits present in the PR but not in master, which should avoid (2).