From ed5b97ea79961f016e92968b169fbe09d9b4189f Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Sat, 4 Nov 2017 22:43:00 +0100 Subject: [PATCH 01/11] Fixing review state to APPROVED whe 'LGTM' in COMMENTED review --- lib/reviews.js | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/reviews.js b/lib/reviews.js index 3456284f..2a0d5db2 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -55,7 +55,7 @@ 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) => { return (isCollaborator(collaborators, r.author)); }).sort((a, b) => { @@ -63,6 +63,7 @@ class ReviewAnalyzer { }); for (const r of list) { + r.state = this.isApprovedInComment(r) ? APPROVED : r.state; const login = r.author.login.toLowerCase(); const entry = map.get(login); if (!entry) { // initialize @@ -97,7 +98,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.hasLGTMInBody(c)) .filter((c) => { return (isCollaborator(collaborators, c.author)); }).sort((a, b) => { @@ -140,6 +141,22 @@ class ReviewAnalyzer { } return result; } + + /** + * @param review + * @returns {boolean} + */ + isApprovedInComment(review) { + return review.state === COMMENTED && this.hasLGTMInBody(review); + } + + /** + * @param object + * @returns {boolean} + */ + hasLGTMInBody(object) { + return LGTM_RE.test(object.bodyText); + } } const REVIEW_SOURCES = { From 4c15b1632bf63046bc904ba9076923e06de23158 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Sun, 5 Nov 2017 01:04:38 +0100 Subject: [PATCH 02/11] Added test case and reworked accordingly --- lib/reviews.js | 19 ++++++++++++------- test/fixtures/reviews_approved.json | 9 +++++++++ 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/lib/reviews.js b/lib/reviews.js index 2a0d5db2..03ff7a0c 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -55,7 +55,7 @@ class ReviewAnalyzer { const map = new Map(); const collaborators = this.collaborators; const list = this.reviews - .filter((r) => r.state !== PENDING) + .filter((r) => r.state !== PENDING || this.isApprovedInComment(r)) .filter((r) => { return (isCollaborator(collaborators, r.author)); }).sort((a, b) => { @@ -63,7 +63,6 @@ class ReviewAnalyzer { }); for (const r of list) { - r.state = this.isApprovedInComment(r) ? APPROVED : r.state; const login = r.author.login.toLowerCase(); const entry = map.get(login); if (!entry) { // initialize @@ -81,6 +80,12 @@ class ReviewAnalyzer { new Review(r.state, r.publishedAt, r.url, FROM_REVIEW) ); break; + case COMMENTED: + map.set( + login, + new Review(r.state, r.publishedAt, r.bodyText, FROM_REVIEW) + ); + break; case DISMISSED: // TODO: check the state of the dismissed review? map.delete(login); @@ -98,7 +103,7 @@ class ReviewAnalyzer { updateMapByRawReviews(oldMap) { const comments = this.comments; const collaborators = this.collaborators; - const withLgtm = comments.filter((c) => this.hasLGTMInBody(c)) + const withLgtm = comments.filter((c) => this.hasLGTM(c, 'bodyText')) .filter((c) => { return (isCollaborator(collaborators, c.author)); }).sort((a, b) => { @@ -133,7 +138,7 @@ class ReviewAnalyzer { const collaborators = this.collaborators; for (const [ login, review ] of reviewers) { const reviewer = collaborators.get(login.toLowerCase()); - if (review.state === APPROVED) { + if (review.state === APPROVED || this.isApprovedInComment(review)) { result.approved.push({ reviewer, review }); } else if (review.state === CHANGES_REQUESTED) { result.rejected.push({ reviewer, review }); @@ -147,15 +152,15 @@ class ReviewAnalyzer { * @returns {boolean} */ isApprovedInComment(review) { - return review.state === COMMENTED && this.hasLGTMInBody(review); + return review.state === COMMENTED && this.hasLGTM(review, 'ref'); } /** * @param object * @returns {boolean} */ - hasLGTMInBody(object) { - return LGTM_RE.test(object.bodyText); + hasLGTM(object, prop) { + return LGTM_RE.test(object[prop]); } } diff --git a/test/fixtures/reviews_approved.json b/test/fixtures/reviews_approved.json index 525cca45..2ab69670 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": "Baz" + }, + "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", + "publishedAt": "2017-10-24T14:49:52Z" +}, { "bodyText": "A few nits", "state": "COMMENTED", From 0992b79bc0bedd5738442d38dc9912bb79700b6a Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Sun, 5 Nov 2017 01:08:34 +0100 Subject: [PATCH 03/11] [SQUASH] added missing comment --- lib/reviews.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/reviews.js b/lib/reviews.js index 03ff7a0c..ef5dba82 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -157,6 +157,7 @@ class ReviewAnalyzer { /** * @param object + * @param prop: string * @returns {boolean} */ hasLGTM(object, prop) { From 37cff9d0f9d1906fbfe5978e2685c5fab0638469 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Sun, 5 Nov 2017 18:17:46 +0100 Subject: [PATCH 04/11] Rebase and rework, approving comments displayed in warning --- lib/metadata_gen.js | 7 ++++++- lib/pr_checker.js | 18 +++++++++++++----- lib/reviews.js | 5 +++-- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/lib/metadata_gen.js b/lib/metadata_gen.js index e1b7caba..80abc0fd 100644 --- a/lib/metadata_gen.js +++ b/lib/metadata_gen.js @@ -2,6 +2,9 @@ const LinkParser = require('./links'); const { EOL } = require('os'); +const { + REVIEW_SOURCES: { FROM_REVIEW } +} = require('./reviews'); /** * @typedef {{reviewer: Collaborator}} Reviewer */ @@ -36,7 +39,9 @@ class MetadataGenerator { `PR-URL: ${prUrl}`, ...fixes.map((fix) => `Fixes: ${fix}`), ...refs.map((ref) => `Refs: ${ref}`), - ...reviewedBy.map((r) => `Reviewed-By: ${r.reviewer.getContact()}`), + ...reviewedBy + .filter((r) => r.source === FROM_REVIEW) + .map((r) => `Reviewed-By: ${r.reviewer.getContact()}`), '' // creates final EOL ]; diff --git a/lib/pr_checker.js b/lib/pr_checker.js index c4a18835..d3d69fa0 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, FROM_REVIEW_COMMENT } } = require('./reviews'); const { FIRST_TIME_CONTRIBUTOR, FIRST_TIMER @@ -65,7 +65,7 @@ class PRChecker { getTSCHint(people) { const tsc = people - .filter((p) => p.reviewer.isTSC()) + .filter((p) => p.review.source === FROM_REVIEW && p.reviewer.isTSC()) .map((p) => p.reviewer.login); let hint = ''; if (tsc.length > 0) { @@ -95,13 +95,21 @@ class PRChecker { status = false; logger.warn(`Approvals: 0`); } else { + let notComm = approved.length; + approved.map((r) => { + if (r.source !== FROM_REVIEW) { notComm--; } + }); let hint = this.getTSCHint(approved); - logger.info(`Approvals: ${approved.length}${hint}`); + logger.info(`Approvals: ${notComm}${hint}`); for (const { reviewer, review } of approved) { if (review.source === FROM_COMMENT) { - logger.info( - `${reviewer.getName()}) approved in via LGTM in comments`); + logger.warn( + `${reviewer.getName()} approved in via LGTM in comments`); + } + if (review.source === FROM_REVIEW_COMMENT) { + logger.warn( + `${reviewer.getName()} approved in via LGTM in commented review`); } } diff --git a/lib/reviews.js b/lib/reviews.js index ef5dba82..ba988a8d 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -7,6 +7,7 @@ const { ascending } = require('./comp'); const LGTM_RE = /(\W|^)lgtm(\W|$)/i; const FROM_REVIEW = 'review'; const FROM_COMMENT = 'comment'; +const FROM_REVIEW_COMMENT = 'review_comment'; class Review { /** @@ -83,7 +84,7 @@ class ReviewAnalyzer { case COMMENTED: map.set( login, - new Review(r.state, r.publishedAt, r.bodyText, FROM_REVIEW) + new Review(r.state, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT) ); break; case DISMISSED: @@ -166,7 +167,7 @@ class ReviewAnalyzer { } const REVIEW_SOURCES = { - FROM_COMMENT, FROM_REVIEW + FROM_COMMENT, FROM_REVIEW, FROM_REVIEW_COMMENT }; module.exports = { From a053af6c2b8dcfcc306ead6d5d650fcfde8e16a4 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Mon, 6 Nov 2017 14:52:49 +0100 Subject: [PATCH 05/11] Rework worklfow + adapt tests to new workflow --- lib/metadata_gen.js | 7 +------ lib/pr_checker.js | 26 ++++++++++++++++++-------- lib/reviews.js | 7 +++++-- test/fixtures/data.js | 2 ++ test/unit/pr_checker.test.js | 2 +- 5 files changed, 27 insertions(+), 17 deletions(-) diff --git a/lib/metadata_gen.js b/lib/metadata_gen.js index 80abc0fd..e1b7caba 100644 --- a/lib/metadata_gen.js +++ b/lib/metadata_gen.js @@ -2,9 +2,6 @@ const LinkParser = require('./links'); const { EOL } = require('os'); -const { - REVIEW_SOURCES: { FROM_REVIEW } -} = require('./reviews'); /** * @typedef {{reviewer: Collaborator}} Reviewer */ @@ -39,9 +36,7 @@ class MetadataGenerator { `PR-URL: ${prUrl}`, ...fixes.map((fix) => `Fixes: ${fix}`), ...refs.map((ref) => `Refs: ${ref}`), - ...reviewedBy - .filter((r) => r.source === FROM_REVIEW) - .map((r) => `Reviewed-By: ${r.reviewer.getContact()}`), + ...reviewedBy.map((r) => `Reviewed-By: ${r.reviewer.getContact()}`), '' // creates final EOL ]; diff --git a/lib/pr_checker.js b/lib/pr_checker.js index d3d69fa0..571bd4fc 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -77,7 +77,7 @@ class PRChecker { checkReviews() { const { - pr, logger, reviewers: { rejected, approved } + pr, logger, reviewers: { rejected, approved, commentApproved } } = this; let status = true; @@ -97,19 +97,15 @@ class PRChecker { } else { let notComm = approved.length; approved.map((r) => { - if (r.source !== FROM_REVIEW) { notComm--; } + if (r.review.source !== FROM_REVIEW) { notComm--; } }); let hint = this.getTSCHint(approved); logger.info(`Approvals: ${notComm}${hint}`); for (const { reviewer, review } of approved) { if (review.source === FROM_COMMENT) { - logger.warn( - `${reviewer.getName()} approved in via LGTM in comments`); - } - if (review.source === FROM_REVIEW_COMMENT) { - logger.warn( - `${reviewer.getName()} approved in via LGTM in commented review`); + logger.info( + `${reviewer.getName()}) approved in via LGTM in comments`); } } @@ -122,6 +118,20 @@ class PRChecker { } } } + if (commentApproved && commentApproved.length !== 0) { + let notComm = approved.length; + approved.map((r) => { + if (r.review.source !== FROM_REVIEW_COMMENT) { notComm--; } + }); + let hint = this.getTSCHint(approved); + logger.info(`LGTM in commented review: ${notComm}${hint}`); + for (const { reviewer, review } of commentApproved) { + if (review.source === FROM_REVIEW_COMMENT) { + logger.info( + `${reviewer.getName()} approved in via LGTM in commented review`); + } + } + } return status; } diff --git a/lib/reviews.js b/lib/reviews.js index ba988a8d..815cce7e 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -134,13 +134,16 @@ class ReviewAnalyzer { const reviewers = this.updateMapByRawReviews(ghReviews); const result = { approved: [], + commentApproved: [], rejected: [] }; const collaborators = this.collaborators; for (const [ login, review ] of reviewers) { const reviewer = collaborators.get(login.toLowerCase()); - if (review.state === APPROVED || this.isApprovedInComment(review)) { - result.approved.push({ reviewer, review }); + if (review.state === APPROVED) { + result.approved.push({reviewer, review}); + } else if (this.isApprovedInComment(review)) { + result.commentApproved.push({reviewer, review}); } else if (review.state === CHANGES_REQUESTED) { result.rejected.push({ reviewer, review }); } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 5c1804ff..9176cf39 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -13,10 +13,12 @@ patchPrototype(rejected, 'review', Review.prototype); const allGreenReviewers = { approved, + commentApproved: [], rejected: [] }; const rejectedReviewers = { rejected, + commentApproved: [], approved: [] }; diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 85766d9f..a1f8975e 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -84,7 +84,7 @@ describe('PRChecker', () => { ], info: [ ['Rejections: 0'], - ['Approvals: 3, 1 from TSC (bar)'], + ['Approvals: 2'], ['Bar User(bar)) approved in via LGTM in comments'] ], error: [], From 0868d7a1ecfd521e337fe9800cc4a1b1f7fb9a6d Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Mon, 6 Nov 2017 14:58:19 +0100 Subject: [PATCH 06/11] Fix bad var name --- lib/pr_checker.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 571bd4fc..9d2f8553 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -119,12 +119,8 @@ class PRChecker { } } if (commentApproved && commentApproved.length !== 0) { - let notComm = approved.length; - approved.map((r) => { - if (r.review.source !== FROM_REVIEW_COMMENT) { notComm--; } - }); let hint = this.getTSCHint(approved); - logger.info(`LGTM in commented review: ${notComm}${hint}`); + logger.info(`LGTM in commented review: ${commentApproved.length}${hint}`); for (const { reviewer, review } of commentApproved) { if (review.source === FROM_REVIEW_COMMENT) { logger.info( From f43df7e70e154215dfaca95e4b1723d940774f0a Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Tue, 7 Nov 2017 10:19:11 +0100 Subject: [PATCH 07/11] Remove rework, change RegExp to stricter one --- lib/pr_checker.js | 22 ++++------------------ lib/reviews.js | 9 +++------ test/fixtures/data.js | 2 -- test/unit/pr_checker.test.js | 2 +- 4 files changed, 8 insertions(+), 27 deletions(-) diff --git a/lib/pr_checker.js b/lib/pr_checker.js index 9d2f8553..c4a18835 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, FROM_REVIEW, FROM_REVIEW_COMMENT } + REVIEW_SOURCES: { FROM_COMMENT } } = require('./reviews'); const { FIRST_TIME_CONTRIBUTOR, FIRST_TIMER @@ -65,7 +65,7 @@ class PRChecker { getTSCHint(people) { const tsc = people - .filter((p) => p.review.source === FROM_REVIEW && p.reviewer.isTSC()) + .filter((p) => p.reviewer.isTSC()) .map((p) => p.reviewer.login); let hint = ''; if (tsc.length > 0) { @@ -77,7 +77,7 @@ class PRChecker { checkReviews() { const { - pr, logger, reviewers: { rejected, approved, commentApproved } + pr, logger, reviewers: { rejected, approved } } = this; let status = true; @@ -95,12 +95,8 @@ class PRChecker { status = false; logger.warn(`Approvals: 0`); } else { - let notComm = approved.length; - approved.map((r) => { - if (r.review.source !== FROM_REVIEW) { notComm--; } - }); let hint = this.getTSCHint(approved); - logger.info(`Approvals: ${notComm}${hint}`); + logger.info(`Approvals: ${approved.length}${hint}`); for (const { reviewer, review } of approved) { if (review.source === FROM_COMMENT) { @@ -118,16 +114,6 @@ class PRChecker { } } } - if (commentApproved && commentApproved.length !== 0) { - let hint = this.getTSCHint(approved); - logger.info(`LGTM in commented review: ${commentApproved.length}${hint}`); - for (const { reviewer, review } of commentApproved) { - if (review.source === FROM_REVIEW_COMMENT) { - logger.info( - `${reviewer.getName()} approved in via LGTM in commented review`); - } - } - } return status; } diff --git a/lib/reviews.js b/lib/reviews.js index 815cce7e..fd20ad03 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -4,7 +4,7 @@ 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'; @@ -134,16 +134,13 @@ class ReviewAnalyzer { const reviewers = this.updateMapByRawReviews(ghReviews); const result = { approved: [], - commentApproved: [], rejected: [] }; const collaborators = this.collaborators; for (const [ login, review ] of reviewers) { const reviewer = collaborators.get(login.toLowerCase()); - if (review.state === APPROVED) { + if (review.state === APPROVED || this.isApprovedInComment(review)) { result.approved.push({reviewer, review}); - } else if (this.isApprovedInComment(review)) { - result.commentApproved.push({reviewer, review}); } else if (review.state === CHANGES_REQUESTED) { result.rejected.push({ reviewer, review }); } @@ -165,7 +162,7 @@ class ReviewAnalyzer { * @returns {boolean} */ hasLGTM(object, prop) { - return LGTM_RE.test(object[prop]); + return LGTM_RE.test(object[prop].trim()); } } diff --git a/test/fixtures/data.js b/test/fixtures/data.js index 9176cf39..5c1804ff 100644 --- a/test/fixtures/data.js +++ b/test/fixtures/data.js @@ -13,12 +13,10 @@ patchPrototype(rejected, 'review', Review.prototype); const allGreenReviewers = { approved, - commentApproved: [], rejected: [] }; const rejectedReviewers = { rejected, - commentApproved: [], approved: [] }; diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index a1f8975e..85766d9f 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -84,7 +84,7 @@ describe('PRChecker', () => { ], info: [ ['Rejections: 0'], - ['Approvals: 2'], + ['Approvals: 3, 1 from TSC (bar)'], ['Bar User(bar)) approved in via LGTM in comments'] ], error: [], From b9b6608cb040c316d4e2fb99a60fde906c117e0c Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Tue, 7 Nov 2017 15:27:57 +0100 Subject: [PATCH 08/11] Add tests cases and comments flag --- lib/args.js | 10 ++++++++-- lib/pr_checker.js | 19 +++++++++++-------- lib/reviews.js | 21 ++++++++++++++------- steps/metadata.js | 2 +- test/fixtures/README/README.md | 2 ++ test/fixtures/collaborators.json | 6 ++++++ test/fixtures/reviewers_approved.json | 14 ++++++++++++++ test/fixtures/reviews_approved.json | 2 +- test/unit/args.test.js | 1 + test/unit/metadata_gen.test.js | 1 + test/unit/pr_checker.test.js | 7 ++++--- 11 files changed, 63 insertions(+), 22 deletions(-) diff --git a/lib/args.js b/lib/args.js index 6d3c2d6b..1c8bb7f3 100644 --- a/lib/args.js +++ b/lib/args.js @@ -37,6 +37,12 @@ function buildYargs(args = null) { describe: 'File to write the metadata in', type: 'string' }) + .option('comments', { + alias: 'c', + demandOption: false, + describe: 'Check for\'LGTM\' in comments', + type: 'boolean' + }) .help() .alias('help', 'h') .argv; @@ -47,8 +53,8 @@ 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, comments } = args; + const result = { owner, repo, file, comments }; if (!isNaN(identifier)) { result.prid = +identifier; } else { diff --git a/lib/pr_checker.js b/lib/pr_checker.js index c4a18835..fd016318 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; @@ -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 fd20ad03..68d11b1b 100644 --- a/lib/reviews.js +++ b/lib/reviews.js @@ -56,7 +56,14 @@ class ReviewAnalyzer { const map = new Map(); const collaborators = this.collaborators; const list = this.reviews - .filter((r) => r.state !== PENDING || this.isApprovedInComment(r)) + .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) => { @@ -84,7 +91,7 @@ class ReviewAnalyzer { case COMMENTED: map.set( login, - new Review(r.state, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT) + new Review(APPROVED, r.publishedAt, r.bodyText, FROM_REVIEW_COMMENT) ); break; case DISMISSED: @@ -104,7 +111,7 @@ class ReviewAnalyzer { updateMapByRawReviews(oldMap) { const comments = this.comments; const collaborators = this.collaborators; - const withLgtm = comments.filter((c) => this.hasLGTM(c, 'bodyText')) + const withLgtm = comments.filter((c) => this.hasLGTM(c)) .filter((c) => { return (isCollaborator(collaborators, c.author)); }).sort((a, b) => { @@ -139,7 +146,7 @@ class ReviewAnalyzer { const collaborators = this.collaborators; for (const [ login, review ] of reviewers) { const reviewer = collaborators.get(login.toLowerCase()); - if (review.state === APPROVED || this.isApprovedInComment(review)) { + if (review.state === APPROVED) { result.approved.push({reviewer, review}); } else if (review.state === CHANGES_REQUESTED) { result.rejected.push({ reviewer, review }); @@ -153,7 +160,7 @@ class ReviewAnalyzer { * @returns {boolean} */ isApprovedInComment(review) { - return review.state === COMMENTED && this.hasLGTM(review, 'ref'); + return review.state === COMMENTED && this.hasLGTM(review); } /** @@ -161,8 +168,8 @@ class ReviewAnalyzer { * @param prop: string * @returns {boolean} */ - hasLGTM(object, prop) { - return LGTM_RE.test(object[prop].trim()); + hasLGTM(object) { + return LGTM_RE.test(object.bodyText.trim()); } } diff --git a/steps/metadata.js b/steps/metadata.js index 6f249c41..d36fb031 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.comments); 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 2ab69670..c17f4a7d 100644 --- a/test/fixtures/reviews_approved.json +++ b/test/fixtures/reviews_approved.json @@ -39,7 +39,7 @@ "bodyText": "LGTM", "state": "COMMENTED", "author": { - "login": "Baz" + "login": "Quux" }, "url": "https://github.com/nodejs/node/pull/16438#pullrequestreview-71817236", "publishedAt": "2017-10-24T14:49:52Z" diff --git a/test/unit/args.test.js b/test/unit/args.test.js index f5de2441..111e861e 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 = { + comments: 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..5c130057 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); }); From 42113f16b781c2962373df5e6a8fa58bb495a194 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Wed, 8 Nov 2017 10:03:41 +0100 Subject: [PATCH 09/11] Changed flag, removed unnecessary parens --- lib/args.js | 9 +++++---- lib/pr_checker.js | 4 ++-- steps/metadata.js | 2 +- test/unit/args.test.js | 2 +- test/unit/pr_checker.test.js | 8 ++++---- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/lib/args.js b/lib/args.js index 1c8bb7f3..086a4175 100644 --- a/lib/args.js +++ b/lib/args.js @@ -37,8 +37,7 @@ function buildYargs(args = null) { describe: 'File to write the metadata in', type: 'string' }) - .option('comments', { - alias: 'c', + .option('check-comments', { demandOption: false, describe: 'Check for\'LGTM\' in comments', type: 'boolean' @@ -53,8 +52,10 @@ const PR_RE = new RegExp( '([0-9]+)(?:/(?:files)?)?$'); function checkAndParseArgs(args) { - const { owner = 'nodejs', repo = 'node', identifier, file, comments } = args; - const result = { owner, repo, file, comments }; + 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 fd016318..26166f8f 100644 --- a/lib/pr_checker.js +++ b/lib/pr_checker.js @@ -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) { @@ -103,7 +103,7 @@ class PRChecker { if (review.source === FROM_COMMENT || review.source === FROM_REVIEW_COMMENT) { logger.warn( - `${reviewer.getName()}) approved in via LGTM in comments`); + `${reviewer.getName()} approved in via LGTM in comments`); } } } diff --git a/steps/metadata.js b/steps/metadata.js index d36fb031..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(argv.comments); + const status = checker.checkAll(argv.checkComments); return { status, request, diff --git a/test/unit/args.test.js b/test/unit/args.test.js index 111e861e..ad699cee 100644 --- a/test/unit/args.test.js +++ b/test/unit/args.test.js @@ -4,7 +4,7 @@ const parseArgs = require('../../lib/args'); const assert = require('assert'); const expected = { - comments: false, + checkComments: false, owner: `nodejs`, repo: `node`, prid: 16637, diff --git a/test/unit/pr_checker.test.js b/test/unit/pr_checker.test.js index 5c130057..a7a2669f 100644 --- a/test/unit/pr_checker.test.js +++ b/test/unit/pr_checker.test.js @@ -80,8 +80,8 @@ describe('PRChecker', () => { const expectedLogs = { warn: [ - ['Quux User(Quux)) approved in via LGTM in comments'], - ['Bar User(bar)) approved in via LGTM in comments'], + ['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: [ @@ -112,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: [], From 3e5c88a0f0be14a78fbbde64c2a2faacce5d6f02 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Wed, 8 Nov 2017 15:15:11 +0100 Subject: [PATCH 10/11] Update README.md for new option --- README.md | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 03514684..327e772b 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: From f3f4afb8768478da546a8c2b4159cb2101320088 Mon Sep 17 00:00:00 2001 From: Benjamin Zaslavsky Date: Wed, 8 Nov 2017 16:17:40 +0100 Subject: [PATCH 11/11] Few nits! --- README.md | 2 +- lib/args.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 327e772b..9c0c41d2 100644 --- a/README.md +++ b/README.md @@ -56,7 +56,7 @@ Options: --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] + --check-comments Check for 'LGTM' in comments [boolean] --help, -h Show help [boolean] ``` diff --git a/lib/args.js b/lib/args.js index 086a4175..6f085d0c 100644 --- a/lib/args.js +++ b/lib/args.js @@ -39,7 +39,7 @@ function buildYargs(args = null) { }) .option('check-comments', { demandOption: false, - describe: 'Check for\'LGTM\' in comments', + describe: 'Check for \'LGTM\' in comments', type: 'boolean' }) .help()