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

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

Open
ivanpauno opened this issue Dec 10, 2021 · 2 comments
Open

Comments

@ivanpauno
Copy link
Contributor

ivanpauno commented Dec 10, 2021

I have seen two differece instances of the issue:

screenshot

image

image

screenshot

image

image

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
Copy link
Contributor Author

cc @cottsay

@ivanpauno ivanpauno changed the title Repo check test is checking the wrong lines Repo check test is sometimes checking or reporting in the wrong lines Dec 10, 2021
@ivanpauno ivanpauno changed the title Repo check test is sometimes checking or reporting in the wrong lines Repo check test is sometimes checking or reporting errors in the wrong lines Dec 10, 2021
@cottsay
Copy link
Member

cottsay commented Dec 14, 2021

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).

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

No branches or pull requests

3 participants