Check run and Pull Reuqest #24872
-
It seems to me that when a check run is created, it referenced the based and target sha. So for each commit on a branch, we have several check runs that are being shown. However, when I create a pull request for that branch, only the results from the last check run is being surfaced and shown. If I want to show all the check runs being produced on that pull request, I need to look at each commit individually. So the question is: I would like to have the ability to see per commit but I would also live to have the pull request to reflect the globality of all the check runs. That will then give a good overview to the user of all the issues that have been surfaced on all the code modified during that code review. How can I achieve that? Thanks! |
Beta Was this translation helpful? Give feedback.
Replies: 11 comments
-
I’m not sure I understand what it is you’re trying to achieve. Let me describe a situation and see if I’m understanding correctly:
All of this can be seen on the pull request in the timeline view and details for each failure can be obtained by clicking the red X. How would surfacing the fact that there were failures that have been fixed be better communicated? For example, on this pull request. |
Beta Was this translation helpful? Give feedback.
-
Let’s take the example mentioned and I will add some additional information in bold:
So there is the issue: at each commit on a branch, a check suite is created. The check suite is only the diff between the previous change and the current one. And check_run are supposed to be added to the check suite. These check run are associated with the sha of the change. And *also* surfaced in the pull request view. In other words: the pull request contains check runs that can be related only to one commit (check run coming from the check suite) but also contains check runs from the pull request. The problem is that all check runs (from check suite AND pull requests) are surfaced on the same view, giving incorrect results (these checks do not apply to the same scope of change, one taking only one commit, another takes multiple). So my question are rather:
Thanks. |
Beta Was this translation helpful? Give feedback.
-
I feel like you may be misunderstanding how Git works. A commit hash, or SHA, refers to a commit in the repository. A commit in the repository contains the state of the entire repository at that point in time, it does not reference a difference between two states. Older source control systems, such as SVN and CVS worked on a diff-based system like you describe. You can still get the difference between two commits out of Git, but that is not what is stored in Git or what is represented by a commit. So check suites and check runs are created for specific commits, not the difference between two commits, as stated in the documentation. To answer your questions: How are check suites and check runs associated to a pull request? By strict definition, they’re not. They’re associated to single commits. Some commits are associated with pull requests and there are visualizations on the PR timeline view for those check suites. What event and data to use to analyze a pull request? The way that GitHub is designed is that the result state of the list of check suites executed against the last commit of the pull request is the state of the pull request itself. This is because when the pull request is merged, the repository will either equal the last commit of the pull request, or very closely resemble it (in the case that there have been changes on the default branch since the pull request branch was created). So the state of the check suites executed against the last commit is the most valid predictor of the success or failure of the pull request after merging. The state of the check suites executed against any previous commit are no longer valid predictors because any problems in those previous commits could have been fixed in the latest commit. This is why the state of the check suites of the last commit are surfaced in the GitHub Pull Request UI: Pull Request from the above screenshot In the example above, when So, to summarize:
I hope that helps and let me know if you have any more questions. |
Beta Was this translation helpful? Give feedback.
-
Thank you for your detailed answer. I will read/process it and reply later. |
Beta Was this translation helpful? Give feedback.
-
First of all, thanks a lot for taking the time to make such a detailed reply. I will then explain my case study so that it will be more clear. What I am doing is a code review tool: I want to make sure that the code that is shipped in a pull request is good. So the code review reviews all the code being shipped/written in a code review, surfaces all the potential problems and ultimately, gives a green/red light for shipping. Therefore, I need to check that the code is good for all the commits for a pull request. Not only the latest one. Let me give a simple example:
The pull request will reflect the status of the last commit only and therefore, shows a pass (because it takes only the value of the latest commit) instead of showing the value of all the commits in the pull requests. Does that explain the problem here? I think the problem is that the check for a code review is that we only want to analyze the subset of code being modified and not the whole repository (and this is what is also specified by a |
Beta Was this translation helpful? Give feedback.
-
Note: Replied with the account that holds the application I develop :slight_smile: |
Beta Was this translation helpful? Give feedback.
-
Yes, I understand your problem description. You want:
The goal of the checks system is to give feedback to the developer of the health of the codebase at the point in time of a given commit. This result can then be used to inform the developer on whether a pull request is ready to be merged or help a team enforce rules around what code is acceptable to be merged into various stages of development (signified by branches). Keeping the above goal in mind, if we somehow surfaced in the pull request UI that the pull request consists of 38 “bad” commits and 42 “good” commits, does this help the developer make the decision of whether or not to press the Merge button? Current Proposed Or maybe I’m misunderstanding your vision of what you want? |
Beta Was this translation helpful? Give feedback.
-
Hi @juli1, The example that you gave doesn’t seem to be correct.
In this example, the check on Step2 will fail unless you have corrected the errors included in code.py in the first step. This is because commits are layered sequentially and don’t stand independent from one another. That’s why only the last commit in a chain is checked. Hope that helps! |
Beta Was this translation helpful? Give feedback.
-
@lee-dohmyou totally understand the use case and got the problem :slight_smile: Many thanks for taking the time to understand the underlying problem, I can understand it is not the common way of using checks! I am not sure what is the “best” solution. Right now, what I am doing is to run check runs on pull-request events: anytime I have a pull-request event, my engine runs and finds issues that have been introduced between the source and target sha. Not sure this is the best way to do it tho, that is why I started this discussion. |
Beta Was this translation helpful? Give feedback.
-
One weakness I can see with this approach is that your engine wouldn’t find issues that existed before the source commit. So let’s say that you enhance your engine to detect new types of problems. Your engine would detect those problems when someone introduced a new one but old ones lingering in the code base would go unnoticed. Another weakness is that sometimes issues in one piece of code can be introduced by changing some other piece of code. A simple example would be that I reference a function Perhaps how you’re analyzing things aren’t subject to these or related pitfalls, though? 🤷♂️ |
Beta Was this translation helpful? Give feedback.
-
You are correct: with such an approach, we do not find previous issues. But that is not the goal: we want to find issues from what is sent. This is the approach taken by usual code review tools, such as phabricator (https://www.phacility.com/phabricator/) or reviewboard (https://www.reviewboard.org/). The reason is the following: somebody might start to install my GitHub app on a old, legacy repository that already has 10,000 issues. No one wants to have all these issues flagged at each code review. Instead, one wants to make sure they are not adding more issues at each commit. This is why we are only inspecting the diff. So I am staying with the current solutions but might still think of integrating the check_suite event when a change is not part of a pull request (for example, when somebody commit a change on a branch without a pull request. Many kudos for your help and time to clarify how check_run and check_suite work |
Beta Was this translation helpful? Give feedback.
One weakness I can see with this approach is that your engine wouldn’t find issues that existed before the source commit. So let’s say that you enhance your engine to detect new types of problems. Your engine would detect those problems when someone introduced a new one but old ones lingering in the code base would go unnoticed.
Another weakness is …