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

Update coverage report to add inline PR comments. #451

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 86 additions & 22 deletions .github/scripts/pr_comment.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,42 +7,35 @@ Update the content of the existing comment.

https://octokit.github.io/rest.js/v19
*/
module.exports = async ({github, context, process}) => {
var octokit_rest = github
module.exports = async ({github, context, process, retry_delay}) => {
// Create the namespace to make it easy to copy/paste example from the
// octokit docs.
const octokit = {rest: github}

const comment_marker = '\n' + process.env.COMMENT_MARKER

if (context.eventName != "pull_request") {
// Only PR are supported.
return
}

var sleep = function(second) {
var sleep = (second) => {
return new Promise(resolve => setTimeout(resolve, second * 1000))
}

/*
Perform the actual logic.

This is wrapped so that we can retry on errors.
Create or update the PR summary report as a single comment.
*/
var doAction = async function() {

console.log(context)

fs = require('fs');

const body = fs.readFileSync(
process.env.GITHUB_WORKSPACE + "/" + process.env.COMMENT_BODY, 'utf8');
var doSummaryComment = async (body) => {
var comment_id = null
var comment_marker = '\n' + process.env.COMMENT_MARKER
var comment_body = body + comment_marker
const comment_body = body + comment_marker

var comments = await octokit_rest.issues.listComments({
const comments = await octokit.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.payload.number,
})

console.log(comments)

comments.data.forEach(comment => {
if (comment.body.endsWith(comment_marker)) {
comment_id = comment.id
Expand All @@ -52,7 +45,7 @@ module.exports = async ({github, context, process}) => {
if (comment_id) {
// We have an existing comment.
// update the content.
await octokit_rest.issues.updateComment({
await octokit.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: comment_id,
Expand All @@ -62,7 +55,7 @@ module.exports = async ({github, context, process}) => {
}

// Create a new comment.
await octokit_rest.issues.createComment({
await octokit.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.payload.number,
Expand All @@ -71,10 +64,81 @@ module.exports = async ({github, context, process}) => {

}

/*
Create or refresh the diff inline comments.
*/
var doDiffComments = async (report) => {

// Start by deleting any previous comments.
const existing_comments = await octokit.rest.pulls.listReviewComments({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.payload.number,
});
existing_comments.data.forEach(async (comment) => {
if (! comment.body.endsWith(comment_marker)) {
// Not our comment.
return
}
await octokit.rest.pulls.deleteReviewComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: comment.id,
})
})

// Prepare inline comments.
var comments = []
Object.keys(report.src_stats).forEach((path) => {
report.src_stats[path].violation_lines.forEach((line) => {
comments.push({
path,
line,
})
})
})

comments.forEach(async (comment) => {
await octokit.rest.pulls.createReviewComment({
owner: context.repo.owner,
repo: context.repo.repo,
commit_id: context.payload.after,
pull_number: context.payload.number,
body: 'Missing coverage.' + comment_marker,
path: comment.path,
line: comment.line,
})
// Wait 1 second to avoid the mitigate the limit.
await sleep(1)
})

}

/*
Perform the actual logic.

This is wrapped so that we can retry on errors.
*/
var doAction = async () => {

console.log(context)

fs = require('fs');

const body = fs.readFileSync(
process.env.GITHUB_WORKSPACE + "/" + process.env.COMMENT_BODY, 'utf8')
await doSummaryComment(body)

const report_json = fs.readFileSync(
process.env.GITHUB_WORKSPACE + "/" + process.env.REPORT_JSON, 'utf8');
await doDiffComments(JSON.parse(report_json))

}

try {
await doAction()
} catch (e) {
await sleep(5)
await sleep(retry_delay)
await doAction()
}
}
1 change: 1 addition & 0 deletions .github/scripts/pr_comment.test.body.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Dummy body of the summary PR comment.
89 changes: 89 additions & 0 deletions .github/scripts/pr_comment.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/*
This should be called via nodejs from the repo root folder.

node .github/scripts/pr_comment.test.js

This is just a poor man's testing code for the script.
Since the project is mostly python, is not easy to add nodeenv and a full
testing suite like jest.
*/
const process = require('process');

process.env.GITHUB_WORKSPACE = process.cwd()
process.env.COMMENT_BODY = '.github/scripts/pr_comment.test.body.txt'
process.env.REPORT_JSON = '.github/scripts/pr_comment.test.report.json'
process.env.COMMENT_MARKER = '<!--- comment-marker -->'

const script = require(process.cwd() + '/.github/scripts/pr_comment.js')
const retry_delay = 0.1

var github = {
issues: {
listComments: (options) => {
console.log('Retrieving comments...')
console.log(options)
return {data: [
{'id': 980, 'body': 'some-other-comment'},
// Comment the line below to trigger creating a new comment.
{'id': 456, 'body': 'comment marker\n<!--- comment-marker -->'},
{'body': 'another-other-comment'},

]}
},
updateComment: (options) => {
console.log('Updating existing comment...')
console.log(options)
},
createComment: (options) => {
console.log('Creating new comment...')
console.log(options)
},
},
pulls: {
listReviews: (options) => {
console.log('Listing existing reviews...')
console.log(options)
return {data: [
{'id': 120, 'body': 'some-other-pr-review'},
// Comment the line below to trigger creating a new comment.
{'id': 156, 'body': 'review marker\n<!--- comment-marker -->'},
]}
},
listReviewComments: (options) => {
console.log('Listing existing review comments...')
console.log(options)
return {data: [
{'id': 120, 'body': 'some-other-pr-review'},
// Comment the line below to trigger creating a new comment.
{'id': 156, 'body': 'review marker\n<!--- comment-marker -->'},
]}
},
createReviewComment: (options) => {
console.log('Creating new review comment...')
console.log(options)
},
deleteReviewComment: (options) => {
console.log('Deleting review comment...')
console.log(options)
},
createReview: (options) => {
console.log('Creating new review...')
console.log(options)
},
},
}

var context = {
eventName: 'pull_request',
repo: {
owner: 'dummy-org',
repo: 'dummy-repo',
},
payload: {
number: 123,
after: '123abc567',
}
}


script({github, context, process, retry_delay})
1 change: 1 addition & 0 deletions .github/scripts/pr_comment.test.report.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{"report_name": "File generate by diff-cover --json-report out.json", "diff_name": "trunk...HEAD, staged and unstaged changes", "src_stats": {"src/towncrier/build.py": {"percent_covered": 22.222222222222214, "violation_lines": [123, 125, 126, 138, 140, 141, 142], "violations": [[123, null], [125, null], [126, null], [138, null], [140, null], [141, null], [142, null]]}}, "total_num_lines": 9, "total_num_violations": 7, "total_percent_covered": 22, "num_changed_lines": 185}
8 changes: 6 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,8 @@ jobs:
# Even we send a comment to a PR, we use the issues API.
# Issues and PR share the same comment API.
issues: write
# This is for sending inline review comments.
pull-requests: write
needs:
# We are waiting only for test jobs.
- test-linux
Expand Down Expand Up @@ -293,7 +295,7 @@ jobs:
echo '```' > coverage-report.txt
coverage report --show-missing >> coverage-report.txt
echo '```' >> coverage-report.txt
diff-cover --markdown-report diff-cover.md --compare-branch origin/trunk coverage.xml
diff-cover --markdown-report diff-cover.md --json-report diff-cover.json --compare-branch origin/trunk coverage.xml
cat diff-cover.md >> coverage-report.txt

# Use the generic JS script to call our custom script
Expand All @@ -303,11 +305,13 @@ jobs:
env:
COMMENT_MARKER: "<!--- automatic-coverage-report -->"
COMMENT_BODY: coverage-report.txt
REPORT_JSON: diff-cover.json
with:
script: |
const script = require(`${process.env.GITHUB_WORKSPACE}/.github/scripts/pr_comment.js`)
// Only pass top level objects as GHA does dependecy injection.
await script({github, context, process})
const retry_delay = 5
await script({github, context, process, retry_delay})

- name: Enforce diff coverage
run: |
Expand Down
13 changes: 13 additions & 0 deletions src/towncrier/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,12 @@ def __main(

click.echo("Finding news fragments...", err=to_err)

if directory == "sdfafsdfasd":
bla = "some line not covered"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing coverage.

# a comment
bla = "some line not covered"
bla = "some line not covered"

if config.directory is not None:
fragment_base_directory = os.path.abspath(config.directory)
fragment_directory = None
Expand All @@ -128,6 +134,13 @@ def __main(
)
fragment_directory = "newsfragments"

if directory == "sdfasdf":
bla = "some line not covered"
# a comment
bla = "some line not covered"
bla = "some line not covered"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing coverage.

fragment_directory = bla

fragment_contents, fragment_filenames = find_fragments(
fragment_base_directory,
config.sections,
Expand Down
Empty file.