diff --git a/README.md b/README.md index 03514684..9c0c41d2 100644 --- a/README.md +++ b/README.md @@ -52,11 +52,12 @@ get-metadata Retrieves metadata for a PR and validates them against nodejs/node PR rules Options: - --version Show version number [boolean] - --owner, -o GitHub owner of the PR repository [string] - --repo, -r GitHub repository of the PR [string] - --file, -f File to write the metadata in [string] - --help, -h Show help [boolean] + --version Show version number [boolean] + --owner, -o GitHub owner of the PR repository [string] + --repo, -r GitHub repository of the PR [string] + --file, -f File to write the metadata in [string] + --check-comments Check for 'LGTM' in comments [boolean] + --help, -h Show help [boolean] ``` Examples: diff --git a/lib/args.js b/lib/args.js index 6d3c2d6b..6f085d0c 100644 --- a/lib/args.js +++ b/lib/args.js @@ -37,6 +37,11 @@ function buildYargs(args = null) { describe: 'File to write the metadata in', type: 'string' }) + .option('check-comments', { + demandOption: false, + describe: 'Check for \'LGTM\' in comments', + type: 'boolean' + }) .help() .alias('help', 'h') .argv; @@ -47,8 +52,10 @@ const PR_RE = new RegExp( '([0-9]+)(?:/(?:files)?)?$'); function checkAndParseArgs(args) { - const { owner = 'nodejs', repo = 'node', identifier, file } = args; - const result = { owner, repo, file }; + const { + owner = 'nodejs', repo = 'node', identifier, file, checkComments + } = args; + const result = { owner, repo, file, checkComments }; if (!isNaN(identifier)) { result.prid = +identifier; } else { diff --git a/lib/pr_checker.js b/lib/pr_checker.js index c4a18835..26166f8f 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -11,7 +11,7 @@ const WEEKDAY_WAIT = 48; const WEEKEND_WAIT = 72; const { - REVIEW_SOURCES: { FROM_COMMENT } + REVIEW_SOURCES: { FROM_COMMENT, FROM_REVIEW_COMMENT } } = require('./reviews'); const { FIRST_TIME_CONTRIBUTOR, FIRST_TIMER @@ -45,9 +45,9 @@ class PRChecker { ); } - checkAll() { + checkAll(comments = false) { const status = [ - this.checkReviews(), + this.checkReviews(comments), this.checkCommitsAfterReview(), this.checkPRWait(new Date()), this.checkCI() @@ -75,7 +75,7 @@ class PRChecker { return hint; } - checkReviews() { + checkReviews(comments = false) { const { pr, logger, reviewers: { rejected, approved } } = this; @@ -88,7 +88,7 @@ class PRChecker { let hint = this.getTSCHint(rejected); logger.warn(`Rejections: ${rejected.length}${hint}`); for (const { reviewer, review } of rejected) { - logger.warn(`${reviewer.getName()}) rejected in ${review.ref}`); + logger.warn(`${reviewer.getName()} rejected in ${review.ref}`); } } if (approved.length === 0) { @@ -98,10 +98,13 @@ class PRChecker { let hint = this.getTSCHint(approved); logger.info(`Approvals: ${approved.length}${hint}`); - for (const { reviewer, review } of approved) { - if (review.source === FROM_COMMENT) { - logger.info( - `${reviewer.getName()}) approved in via LGTM in comments`); + if (comments) { + for (const {reviewer, review} of approved) { + if (review.source === FROM_COMMENT || + review.source === FROM_REVIEW_COMMENT) { + logger.warn( + `${reviewer.getName()} approved in via LGTM in comments`); + } } } diff --git a/lib/reviews.js b/lib/reviews.js index 3456284f..68d11b1b 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -4,9 +4,10 @@ const { } = require('./review_state'); const { isCollaborator } = require('./collaborators'); const { ascending } = require('./comp'); -const LGTM_RE = /(\W|^)lgtm(\W|$)/i; +const LGTM_RE = /^lgtm\W?$/i; const FROM_REVIEW = 'review'; const FROM_COMMENT = 'comment'; +const FROM_REVIEW_COMMENT = 'review_comment'; class Review { /** @@ -55,7 +56,14 @@ class ReviewAnalyzer { const map = new Map(); const collaborators = this.collaborators; const list = this.reviews - .filter((r) => r.state !== PENDING && r.state !== COMMENTED) + .filter((r) => r.state !== PENDING) + .filter((r) => { + if (r.state === COMMENTED) { + return this.isApprovedInComment(r); + } else { + return true; + } + }) .filter((r) => { return (isCollaborator(collaborators, r.author)); }).sort((a, b) => { @@ -80,6 +88,12 @@ class ReviewAnalyzer { new Review(r.state, r.publishedAt, r.url, FROM_REVIEW) ); break; + case COMMENTED: + map.set( + login, + new Review(APPROVED, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT) + ); + break; case DISMISSED: // TODO: check the state of the dismissed review? map.delete(login); @@ -97,7 +111,7 @@ class ReviewAnalyzer { updateMapByRawReviews(oldMap) { const comments = this.comments; const collaborators = this.collaborators; - const withLgtm = comments.filter((c) => LGTM_RE.test(c.bodyText)) + const withLgtm = comments.filter((c) => this.hasLGTM(c)) .filter((c) => { return (isCollaborator(collaborators, c.author)); }).sort((a, b) => { @@ -133,17 +147,34 @@ class ReviewAnalyzer { for (const [ login, review ] of reviewers) { const reviewer = collaborators.get(login.toLowerCase()); if (review.state === APPROVED) { - result.approved.push({ reviewer, review }); + result.approved.push({reviewer, review}); } else if (review.state === CHANGES_REQUESTED) { result.rejected.push({ reviewer, review }); } } return result; } + + /** + * @param review + * @returns {boolean} + */ + isApprovedInComment(review) { + return review.state === COMMENTED && this.hasLGTM(review); + } + + /** + * @param object + * @param prop: string + * @returns {boolean} + */ + hasLGTM(object) { + return LGTM_RE.test(object.bodyText.trim()); + } } const REVIEW_SOURCES = { - FROM_COMMENT, FROM_REVIEW + FROM_COMMENT, FROM_REVIEW, FROM_REVIEW_COMMENT }; module.exports = { diff --git a/steps/metadata.js b/steps/metadata.js index 6f249c41..4a15ca1b 100644 --- a/steps/metadata.js +++ b/steps/metadata.js @@ -34,7 +34,7 @@ module.exports = async function getMetadata(argv, logger) { }, 'Generated metadata:'); const checker = new PRChecker(logger, data); - const status = checker.checkAll(); + const status = checker.checkAll(argv.checkComments); return { status, request, diff --git a/test/fixtures/README/README.md b/test/fixtures/README/README.md index e689584f..7c8c81c6 100644 --- a/test/fixtures/README/README.md +++ b/test/fixtures/README/README.md @@ -252,6 +252,8 @@ For more information about the governance of the Node.js project, see **Foo User** <foo@example.com> (she/her) * [Quo](https://github.com/quo) - **Quo User** <quo@example.com> (she/her) +* [Quux](https://github.com/quux) - +**Quux User** <quux@example.com> (he/him) ### Collaborator Emeriti diff --git a/test/fixtures/collaborators.json b/test/fixtures/collaborators.json index 49a5d615..b40e39fe 100644 --- a/test/fixtures/collaborators.json +++ b/test/fixtures/collaborators.json @@ -22,5 +22,11 @@ "name": "Quo User", "email": "quo@example.com", "type": "COLLABORATOR" + }, + { + "login": "Quux", + "name": "Quux User", + "email": "quux@example.com", + "type": "COLLABORATOR" } ] diff --git a/test/fixtures/reviewers_approved.json b/test/fixtures/reviewers_approved.json index 2b0acb84..0f9856ef 100644 --- a/test/fixtures/reviewers_approved.json +++ b/test/fixtures/reviewers_approved.json @@ -13,6 +13,20 @@ "source": "review" } }, + { + "reviewer": { + "login": "Quux", + "name": "Quux User", + "email": "quux@example.com", + "type": "COLLABORATOR" + }, + "review": { + "state": "APPROVED", + "date": "2017-10-24T14:49:52Z", + "ref": "LGTM", + "source": "review_comment" + } + }, { "reviewer": { "login": "Baz", diff --git a/test/fixtures/reviews_approved.json b/test/fixtures/reviews_approved.json index 525cca45..c17f4a7d 100644 --- a/test/fixtures/reviews_approved.json +++ b/test/fixtures/reviews_approved.json @@ -35,6 +35,15 @@ "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71488236", "publishedAt": "2017-10-24T14:49:52Z" }, +{ + "bodyText": "LGTM", + "state": "COMMENTED", + "author": { + "login": "Quux" + }, + "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", + "publishedAt": "2017-10-24T14:49:52Z" +}, { "bodyText": "A few nits", "state": "COMMENTED", diff --git a/test/unit/args.test.js b/test/unit/args.test.js index f5de2441..ad699cee 100644 --- a/test/unit/args.test.js +++ b/test/unit/args.test.js @@ -4,6 +4,7 @@ const parseArgs = require('../../lib/args'); const assert = require('assert'); const expected = { + checkComments: false, owner: `nodejs`, repo: `node`, prid: 16637, diff --git a/test/unit/metadata_gen.test.js b/test/unit/metadata_gen.test.js index f964b224..ddac9039 100644 --- a/test/unit/metadata_gen.test.js +++ b/test/unit/metadata_gen.test.js @@ -19,6 +19,7 @@ const expected = `PR-URL: https://github.com/nodejs/node/pull/16438 Fixes: https://github.com/nodejs/node/issues/16437 Refs: https://github.com/nodejs/node/pull/15148 Reviewed-By: Foo User +Reviewed-By: Quux User Reviewed-By: Baz User Reviewed-By: Bar User `; diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 85766d9f..a7a2669f 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -80,12 +80,13 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ + ['Quux User(Quux) approved in via LGTM in comments'], + ['Bar User(bar) approved in via LGTM in comments'], ['semver-major requires at least two TSC approvals'] ], info: [ ['Rejections: 0'], - ['Approvals: 3, 1 from TSC (bar)'], - ['Bar User(bar)) approved in via LGTM in comments'] + ['Approvals: 4, 1 from TSC (bar)'] ], error: [], trace: [] @@ -100,7 +101,7 @@ describe('PRChecker', () => { collaborators }); - const status = checker.checkReviews(); + const status = checker.checkReviews(true); assert(!status); assert.deepStrictEqual(logger.logs, expectedLogs); }); @@ -111,8 +112,8 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ ['Rejections: 2, 1 from TSC (bar)'], - ['Foo User(foo)) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], - ['Bar User(bar)) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624'], + ['Foo User(foo) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71480624'], + ['Bar User(bar) rejected in https://github.com/nodejs/node/pull/16438#pullrequestreview-71482624'], ['Approvals: 0'] ], info: [],