From dff6ee32725197bdb81f3f63c5bd9805f2ed22bb Mon Sep 17 00:00:00 2001 From: Igor Minar Date: Thu, 19 Jan 2017 16:24:47 -0800 Subject: [PATCH] ci: validate the message of each new commit as part of the CI linting This patch adds the gulp command of `validate-commit-messages` which will validate the range of commits messages present in the active branch. This check now runs on CI as part of the linting checks. Allowed commit message types and scopes are controlled via commit-message.json file and documented at https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines This solution is based on old Vojta's code that he wrote for angular/angular.js, that was later adjusted by @matsko in #13815. Ideally we should switch over to something like https://www.npmjs.com/package/commitplease as suggested in #9953 but that package currently doesn't support strict scope checking, which is one of the primarily goal of this PR. Note that this PR removes support for "chore" which was previously overused by everyone on the team. Closes #13815 Fixes #3337 --- CONTRIBUTING.md | 47 +++++-- gulpfile.js | 36 +++++- scripts/ci-lite/test_js.sh | 4 + .../commit-message.json | 35 ++++++ tools/validate-commit-message/index.js | 1 + .../spec/support/jasmine.json | 11 ++ .../validate-commit-message.js | 52 ++++++++ .../validate-commit-message.spec.js | 119 ++++++++++++++++++ 8 files changed, 293 insertions(+), 12 deletions(-) create mode 100644 tools/validate-commit-message/commit-message.json create mode 100644 tools/validate-commit-message/index.js create mode 100644 tools/validate-commit-message/spec/support/jasmine.json create mode 100644 tools/validate-commit-message/validate-commit-message.js create mode 100644 tools/validate-commit-message/validate-commit-message.spec.js diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 109cd9fc57..92b771b1d5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -191,21 +191,46 @@ If the commit reverts a previous commit, it should begin with `revert: `, follow ### Type Must be one of the following: -* **feat**: A new feature -* **fix**: A bug fix -* **docs**: Documentation only changes -* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing - semi-colons, etc) -* **refactor**: A code change that neither fixes a bug nor adds a feature -* **perf**: A code change that improves performance -* **test**: Adding missing tests or correcting existing tests * **build**: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm) * **ci**: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs) -* **chore**: Other changes that don't modify `src` or `test` files +* **docs**: Documentation only changes +* **feat**: A new feature +* **fix**: A bug fix +* **perf**: A code change that improves performance +* **refactor**: A code change that neither fixes a bug nor adds a feature +* **style**: Changes that do not affect the meaning of the code (white-space, formatting, missing + semi-colons, etc) +* **test**: Adding missing tests or correcting existing tests ### Scope -The scope could be anything specifying place of the commit change. For example -`Compiler`, `ElementInjector`, etc. +The scope should be the name of the npm package affected (as perceived by person reading changelog generated from commit messages. + +The following is the list of supported scopes: + +* **common** +* **compiler** +* **compiler-cli** +* **core** +* **forms** +* **http** +* **language-service** +* **platform-browser** +* **platform-browser-dynamic** +* **platform-server** +* **platform-webworker** +* **platform-webworker-dynamic** +* **router** +* **upgrade** +* **tsc-wrapped** + +There is currently few exception to the "use package name" rule: + +* **packaging**: used for changes that change the npm package layout in all of our packages, e.g. public path changes, package.json changes done to all packages, d.ts file/format changes, changes to bundles, etc. +* **changelog**: used for updating the release notes in CHANGELOG.md +* none/empty string: useful for `style`, `test` and `refactor` changes that are done across all packages (e.g. `style: add missing semicolons`) + + +Packaging ### Subject The subject contains succinct description of the change: diff --git a/gulpfile.js b/gulpfile.js index ff2c446ae0..7ad5da28c5 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -126,7 +126,7 @@ gulp.task('public-api:update', ['build.sh'], (done) => { }); // Check the coding standards and programming errors -gulp.task('lint', ['format:enforce', 'tools:build'], () => { +gulp.task('lint', ['format:enforce', 'tools:build', 'validate-commit-messages'], () => { const tslint = require('gulp-tslint'); // Built-in rules are at // https://palantir.github.io/tslint/rules/ @@ -155,6 +155,40 @@ gulp.task('lint', ['format:enforce', 'tools:build'], () => { .pipe(tslint.report({emitError: true})); }); +gulp.task('validate-commit-messages', () => { + const validateCommitMessage = require('./tools/validate-commit-message'); + const childProcess = require('child_process'); + + // We need to fetch origin explicitly because it might be stale. + // I couldn't find a reliable way to do this without fetch. + childProcess.exec( + 'git fetch origin master && git log --reverse --format=%s HEAD ^origin/master', + (error, stdout, stderr) => { + if (error) { + console.log(stderr); + process.exit(1); + } + + let someCommitsInvalid = false; + let commitsByLine = stdout.trim().split(/\n/); + + console.log(`Examining ${commitsByLine.length} commits between HEAD and master`); + + if (commitsByLine.length == 0) { + console.log('There are zero new commits between this HEAD and master'); + } + + someCommitsInvalid = !commitsByLine.every(validateCommitMessage); + + if (someCommitsInvalid) { + console.log('Please fix the failing commit messages before continuing...'); + console.log( + 'Commit message guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines'); + process.exit(1); + } + }); +}); + gulp.task('tools:build', (done) => { tsc('tools/', done); }); // Check for circular dependency in the source code diff --git a/scripts/ci-lite/test_js.sh b/scripts/ci-lite/test_js.sh index 7f0c22b123..1c7bac81a0 100755 --- a/scripts/ci-lite/test_js.sh +++ b/scripts/ci-lite/test_js.sh @@ -20,6 +20,10 @@ echo 'travis_fold:start:test.unit.tools' # Run unit tests in tools node ./dist/tools/tsc-watch/ tools runCmdsOnly +cd tools/validate-commit-message +$(npm bin)/jasmine +cd - + echo 'travis_fold:end:test.unit.tools' diff --git a/tools/validate-commit-message/commit-message.json b/tools/validate-commit-message/commit-message.json new file mode 100644 index 0000000000..ec19bb7e13 --- /dev/null +++ b/tools/validate-commit-message/commit-message.json @@ -0,0 +1,35 @@ +{ + "maxLength": 100, + "types": [ + "build", + "ci", + "docs", + "feat", + "fix", + "perf", + "refactor", + "style", + "test" + ], + "scopes": [ + "benchpress", + "common", + "compiler", + "compiler-cli", + "core", + "forms", + "http", + "language-service", + "platform-browser", + "platform-browser-dynamic", + "platform-server", + "platform-webworker", + "platform-webworker-dynamic", + "router", + "upgrade", + "tsc-wrapped", + + "packaging", + "changelog" + ] +} diff --git a/tools/validate-commit-message/index.js b/tools/validate-commit-message/index.js new file mode 100644 index 0000000000..7bb7b84edc --- /dev/null +++ b/tools/validate-commit-message/index.js @@ -0,0 +1 @@ +module.exports = require('./validate-commit-message'); \ No newline at end of file diff --git a/tools/validate-commit-message/spec/support/jasmine.json b/tools/validate-commit-message/spec/support/jasmine.json new file mode 100644 index 0000000000..31a39bc6a2 --- /dev/null +++ b/tools/validate-commit-message/spec/support/jasmine.json @@ -0,0 +1,11 @@ +{ + "spec_dir": "", + "spec_files": [ + "**/*[sS]pec.js" + ], + "helpers": [ + "helpers/**/*.js" + ], + "stopSpecOnExpectationFailure": false, + "random": false +} diff --git a/tools/validate-commit-message/validate-commit-message.js b/tools/validate-commit-message/validate-commit-message.js new file mode 100644 index 0000000000..bdfc727bac --- /dev/null +++ b/tools/validate-commit-message/validate-commit-message.js @@ -0,0 +1,52 @@ +#!/usr/bin/env node + +/** + * GIT commit message format enforcement + * + * Note: this script was originally written by Vojta for AngularJS :-) + */ + +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const configPath = path.resolve(__dirname, './commit-message.json'); +const config = JSON.parse(fs.readFileSync(configPath, 'utf8')); +const PATTERN = /^(revert\: )?(\w+)(?:\(([^)]+)\))?\: (.+)$/; + +module.exports = function(commitSubject) { + if (commitSubject.length > config['maxLength']) { + error(`The commit message is longer than ${config['maxLength']} characters`, commitSubject); + return false; + } + + const match = PATTERN.exec(commitSubject); + if (!match || match[2] === 'revert') { + error( + `The commit message does not match the format of "(): OR revert: type(): "`, + commitSubject); + return false; + } + + const type = match[2]; + if (config['types'].indexOf(type) === -1) { + error( + `${type} is not an allowed type.\n => TYPES: ${config['types'].join(', ')}`, commitSubject); + return false; + } + + const scope = match[3]; + + if (scope && !config['scopes'].includes(scope)) { + error( + `"${scope}" is not an allowed scope.\n => SCOPES: ${config['scopes'].join(', ')}`, + commitSubject); + return false; + } + + return true; +}; + +function error(errorMessage, commitMessage) { + console.error(`INVALID COMMIT MSG: "${commitMessage}"\n => ERROR: ${errorMessage}`); +} diff --git a/tools/validate-commit-message/validate-commit-message.spec.js b/tools/validate-commit-message/validate-commit-message.spec.js new file mode 100644 index 0000000000..bb78633904 --- /dev/null +++ b/tools/validate-commit-message/validate-commit-message.spec.js @@ -0,0 +1,119 @@ +'use strict'; + +describe('validate-commit-message.js', function() { + var validateMessage = require('./validate-commit-message'); + var errors = []; + var logs = []; + + var VALID = true; + var INVALID = false; + + beforeEach(function() { + errors.length = 0; + logs.length = 0; + + spyOn(console, 'error').and.callFake(function(msg) { + errors.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor + }); + + spyOn(console, 'log').and.callFake(function(msg) { + logs.push(msg.replace(/\x1B\[\d+m/g, '')); // uncolor + }); + }); + + describe('validateMessage', function() { + + it('should be valid', function() { + expect(validateMessage('fix(core): something')).toBe(VALID); + expect(validateMessage('feat(common): something')).toBe(VALID); + expect(validateMessage('docs(compiler): something')).toBe(VALID); + expect(validateMessage('style(http): something')).toBe(VALID); + expect(validateMessage('refactor(platform-webworker): something')).toBe(VALID); + expect(validateMessage('test(language-service): something')).toBe(VALID); + expect(validateMessage('test(packaging): something')).toBe(VALID); + expect(errors).toEqual([]); + }); + + + it('should fail when scope is invalid', function() { + expect(validateMessage('fix(Compiler): something')).toBe(INVALID); + expect(validateMessage('feat(bah): something')).toBe(INVALID); + expect(validateMessage('docs(animations): something')).toBe(INVALID); + expect(validateMessage('style(webworker): something')).toBe(INVALID); + expect(validateMessage('refactor(security): something')).toBe(INVALID); + expect(validateMessage('refactor(docs): something')).toBe(INVALID); + ['INVALID COMMIT MSG: "fix(Compiler): something"\n' + + ' => ERROR: "Compiler" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "feat(bah): something"\n' + + ' => ERROR: "bah" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "docs(animations): something"\n' + + ' => ERROR: "animations" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "style(webworker): something"\n' + + ' => ERROR: "webworker" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "refactor(security): something"\n' + + ' => ERROR: "security" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog', + 'INVALID COMMIT MSG: "refactor(docs): something"\n' + + ' => ERROR: "docs" is not an allowed scope.\n' + + ' => SCOPES: benchpress, common, compiler, compiler-cli, core, forms, http, language-service, platform-browser, platform-browser-dynamic, platform-server, platform-webworker, platform-webworker-dynamic, router, upgrade, tsc-wrapped, packaging, changelog'] + .forEach((expectedErrorMessage, index) => { + expect(expectedErrorMessage).toEqual(errors[index]); + }); + }); + + + it('should validate 100 characters length', function() { + var msg = + 'fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... '; + + expect(validateMessage(msg)).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "fix(compiler): something super mega extra giga tera long, maybe even longer and longer and longer... "\n => ERROR: The commit message is longer than 100 characters' + ]); + }); + + + it('should validate "(): " format', function() { + var msg = 'not correct format'; + + expect(validateMessage(msg)).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "not correct format"\n => ERROR: The commit message does not match the format of "(): OR revert: type(): "' + ]); + }); + + + it('should support "revert: type(scope):" syntax and reject "revert(scope):" syntax', function() { + let correctMsg = 'revert: fix(compiler): reduce generated code payload size by 65%'; + expect(validateMessage(correctMsg)).toBe(VALID); + + let incorretMsg = 'revert(compiler): reduce generated code payload size by 65%'; + expect(validateMessage(incorretMsg)).toBe(INVALID); + expect(errors).toEqual([ + 'INVALID COMMIT MSG: "revert(compiler): reduce generated code payload size by 65%"\n => ERROR: The commit message does not match the format of "(): OR revert: type(): "' + ]); + }); + + + it('should validate type', function() { + expect(validateMessage('weird($filter): something')).toBe(INVALID); + expect(errors).toEqual( + ['INVALID COMMIT MSG: "weird($filter): something"\n' + + ' => ERROR: weird is not an allowed type.\n' + + ' => TYPES: build, ci, docs, feat, fix, perf, refactor, style, test']); + }); + + + it('should allow empty scope', + function() { expect(validateMessage('fix: blablabla')).toBe(VALID); }); + + // we don't want to allow WIP. it's ok to fail the PR build in this case to show that there is + // work still to be done. + it('should not ignore msg prefixed with "WIP: "', + function() { expect(validateMessage('WIP: bullshit')).toBe(INVALID); }); + }); +}); \ No newline at end of file