test: Changed changelog linting to check the pull request title #704

rpl · 2016-12-22T20:31:19Z

This pull request contains an attempt to implement a better changelog linting by running conventiona-changelog-lint on the pull request title instead of running it on the the commit messages, especially given that the single commits are going to be squashed when the pull request is merged, while the pull request title is what becomes the new commit message and it wasn't currently linted using conventional-changelog-lint, until it reaches master (which means... too late :-))

Fixes #701

rpl · 2016-12-22T20:34:02Z

@kumar303 This pull request currently contains an "invalid pull request title" on purpose, so that we can test that the related travis build fails, and then we can change the title and force a rebuild to test that it passes when the pull request title follows the conventions correctly.

coveralls · 2016-12-22T20:36:05Z

Coverage remained the same at 100.0% when pulling 17fb45d on rpl:test/changelog-lint-pullrequest-title into dbdf9e2 on mozilla:master.

rpl · 2016-12-22T20:42:29Z

@kumar303 Here is a link to the expected failure: https://travis-ci.org/mozilla/web-ext/jobs/186182234#L1601-L1634

coveralls · 2016-12-22T20:43:46Z

Coverage remained the same at 100.0% when pulling 0d636c4 on rpl:test/changelog-lint-pullrequest-title into dbdf9e2 on mozilla:master.

kumar303

Nice! I feel like one day we should move all grunt scripts to plain on npm run scripts but, hey, that day might never come.

I requested some minor changes and I also think you'll need to incorporate the logic from #701 (comment) if it's not too tricky.

kumar303 · 2016-12-22T20:39:12Z

.travis.yml


 before_script:
 # If this command fails and you can't fix it, file an issue and add an exception to .nsprc
 - npm run nsp-check

 script:
 - COVERAGE=y NODE_ENV=production npm test
- npm run changelog-lint
+- npm run travis-pullrequest-title-lint


How about github-pr-title-lint ?

kumar303 · 2016-12-22T20:40:54Z

Gruntfile.js

+          pullRequestURLPath
+        );
+
+        require('https').get({


maybe move this require to the top of the file?

kumar303 · 2016-12-22T20:43:31Z

Gruntfile.js

+            done(false);
+          });
+          response.on('end', function() {
+            var parsed = JSON.parse(body);


it would be good to start the promise chain with this wrapped in new Promise(function(resolve) { ... }) just in case it throws a SyntaxError. Otherwise, the grunt task might hang.

kumar303 · 2016-12-22T20:52:41Z

It would be nice to get a more readable report of errors, if possible. The standard changelog lint output is already pretty confusing to contributors though. Perhaps this should also include a link to the formatting guide ?

coveralls · 2016-12-22T23:01:44Z

Coverage remained the same at 100.0% when pulling 2e95f3d on rpl:test/changelog-lint-pullrequest-title into dbdf9e2 on mozilla:master.

rpl · 2016-12-22T23:07:24Z

@kumar303 Here is the new failure messages (formatted as the cli tool does and with a nice message added, which links to the "Writing commit messages" section in the CONTRIBUTING.md docs):

https://travis-ci.org/mozilla/web-ext/jobs/186214389#L1602-L1618

The name of the task is still travis-... because, at least in its current implementation, it is supposed to work correctly only on travis (while locally you have to define the needed environment vars manually to be able to use it).

I opted to move the new task into its own file, it looks like "a good time to move some complex tasks into their own file" can be now after all ;-)

I also changed the task so that it is going to lint the commit message if the branch is composed from a single commit.

Let me know how it looks to you.

kumar303

This looks great! Most nits but I was also unsure about the template string.

kumar303 · 2016-12-23T16:44:56Z

Gruntfile.js

@@ -13,7 +13,7 @@ module.exports = function(grunt) {

  var configs = require('load-grunt-configs')(grunt, {
    config: {
-      src: 'tasks/*.js',
+      src: 'tasks/configs/*.js',


Good idea. The helper comment needs updating too: https://github.com/rpl/web-ext/blob/2e95f3dd3db4df77177c8a459965d61831860425/Gruntfile.js#L8

kumar303 · 2016-12-23T16:49:39Z

tasks/travis-pr-title-lint.js

+  function getPullRequestTitle() {
+    return new Promise(function(resolve, reject) {
+      var pullRequestURLPath = `/repos/${ process.env.TRAVIS_REPO_SLUG
+        }${/pulls/ }${process.env.TRAVIS_PULL_REQUEST }.json`;


Is it ok to use a template string here? I'm surprised this works.

ouch, it shouldn't be there, I tried to remove any ES6 syntax to keep it "ES5"-safe :-)
but I forgot a bunch of them (but it looks like template strings are supported in node >= 4).

removed in the updated pull request.

kumar303 · 2016-12-23T16:51:31Z

tasks/travis-pr-title-lint.js

+  function getPullRequestTitle() {
+    return new Promise(function(resolve, reject) {
+      var pullRequestURLPath = `/repos/${ process.env.TRAVIS_REPO_SLUG
+        }${/pulls/ }${process.env.TRAVIS_PULL_REQUEST }.json`;


nit: I find these easier to read when broken with plus signs (but I know it's not pretty either way):

var pullRequestURLPath = '/repos/' + `${process.env.TRAVIS_REPO_SLUG}${/pulls/}` + `${process.env.TRAVIS_PULL_REQUEST }.json`;

is ${/pulls/} correct?

it shouldn't, and it is very interesting that it worked (and it didn't raise any linting error)
I rewrote all the template strings into strings composition, but I will take another look on this (at least to prevent this kind of weird but working template strings to slip in a patch unnoticed in the future ;-)).

kumar303 · 2016-12-23T16:54:28Z

tasks/travis-pr-title-lint.js

+  }
+
+  var writeCommitMessagesDocURL =
+        'https://github.com/mozilla/web-ext/blob/master/CONTRIBUTING.md#writing-commit-messages'; // eslint-disable-line max-len


nit: we've been doing two spaces for any indent, regardless of the type of block continuation. I know some editors do these breaks with more spaces by default (mine does).

yeah, this will be definitely in a chapter of my book "when your editor auto-indentation feature works against you" :-D

when I moved all the grunt task code into a separate file, the "auto-indentation on copy & paste" feature decided to mess up my manual indentation :-|

kumar303 · 2016-12-23T16:56:19Z

tasks/travis-pr-title-lint.js

+          if (commitsCount === 1) {
+            grunt.log.writeln(
+              'There is only one commit in this pull request, ' +
+                'we are going to check the single commit message...'


nit: typically we've been indenting breaks like this at the same level since it's not a nested block, it's a continuation of the block on the previous line

kumar303 · 2016-12-23T16:57:23Z

tasks/travis-pr-title-lint.js

+              'There is more than one commit in this pull request, ' +
+                'we are going to check the pull request title...'
+            );
+            return getPullRequestTitle().then(lintMessage);


It looks like .then(lintMessage) is the same for both cases so it can be moved down into the promise chain

kumar303 · 2016-12-23T16:58:16Z

tasks/travis-pr-title-lint.js

+          );
+
+          grunt.log.writeln(
+            `${'\nDon\'t panic! If your travis build is failing here, ' +


I love this message!

hehehe, I knew you would like it!

Every book should have "Don't panic" on its cover ;-)

kumar303 · 2016-12-23T16:59:40Z

tasks/travis-pr-title-lint.js

+              'please take a look at \n\n' +
+              ' - '}${ writeCommitMessagesDocURL }\n\n` +
+              'and/or mention in a comment one of the mantainers, ' +
+              'we are here to help ;-)'


I think you could just say:

Please take a look at [link] and feel free to ask for help from one of the maintainers in a comment; we are here to help ;-)

I'm suggesting that because they may not know what usernames to mention and I think we are pretty good about reading all comments anyway.

👍
message tweaked in the last pushed update!

coveralls · 2016-12-23T17:36:10Z

Coverage remained the same at 100.0% when pulling fc0ad41 on rpl:test/changelog-lint-pullrequest-title into dbdf9e2 on mozilla:master.

rpl · 2016-12-23T17:45:11Z

@kumar303 The last push build passed, and it shouldn't until we change the pull request title.

do not merge this pull request yet, I'm looking into it

coveralls · 2016-12-24T04:32:16Z

Coverage remained the same at 100.0% when pulling e1d06d5 on rpl:test/changelog-lint-pullrequest-title into 31d2e46 on mozilla:master.

coveralls · 2016-12-24T05:09:59Z

Coverage remained the same at 100.0% when pulling acbc022 on rpl:test/changelog-lint-pullrequest-title into 31d2e46 on mozilla:master.

coveralls · 2016-12-24T05:31:21Z

Coverage remained the same at 100.0% when pulling 9c7c0fc on rpl:test/changelog-lint-pullrequest-title into 31d2e46 on mozilla:master.

rpl · 2016-12-24T06:33:05Z

@kumar303 I tweaked the grunt task a bit:

removed more es6 syntaxes (template strings and arrow functions)
explicitly check and reject on empty message (which should prevent a "wrong" green build like this: https://travis-ci.org/mozilla/web-ext/jobs/186392172#L1117)
added an explicit "catch & reject" in the "changelog-lint"-related promise chain
quoted the changelog-lint input (so that it is clear where it starts and where it ends ;-))
removed git-hooks from the npm dependencies

I've not yet removed the .githooks dir because it seems to me that, on a local dev environment of a contributor where the git hooks are already installed, the raised error message (the one that would be raised if the .githooks dir is missing) can be confusing, and so it is probably better to defer their removal (but in the meantime it is reasonable that we do not auto-install the git hooks anymore).

I played a bit with this branch by rebuilding it on travis multiple times, with and without changing the title,
and it looks to behave correctly, I'm going to do it one more time so that it goes green with a valid pull request title.

Once the travis build is green, it should be ready, let me know how it looks to you.

coveralls · 2016-12-26T06:26:00Z

Coverage remained the same at 100.0% when pulling cac5926 on rpl:test/changelog-lint-pullrequest-title into 31d2e46 on mozilla:master.

rpl · 2016-12-26T06:29:39Z

@kumar303 I updated the CONTRIBUTING.md file so that it mentions that the pull requests subject is checked using the same approach used for the commit messages (The new changelog linting task actually checks the commit messages only when there is only one commit in the pull request, but I preferred to keep it simple and don't mention this behavior in the CONTRIBUTING.md change).

kumar303

This looks great, is it ready to merge? If so, merge it in!

Here's one idea about the .githooks. What if you keep the scripts there but make them do nothing? They could also print "This commit hook is no longer needed, you can safely remove it." At the very least, this patch should probably add a comment in each hook script about the change.

coveralls · 2016-12-28T01:35:45Z

Coverage remained the same at 100.0% when pulling 63b55a5 on rpl:test/changelog-lint-pullrequest-title into 31d2e46 on mozilla:master.

rpl · 2016-12-28T03:13:50Z

Here's one idea about the .githooks. What if you keep the scripts there but make them do nothing? They could also print "This commit hook is no longer needed, you can safely remove it." At the very least, this patch should probably add a comment in each hook script about the change.

👍 sounds great to me!

I applied it in 63b55a5

test: run changelog-lint on the pull request title on travis build.

17fb45d

rpl requested a review from kumar303 December 22, 2016 20:31

muffinresearch added the state: pull request ready label Dec 22, 2016

fix: change 'ES6 syntaxes' not supported on node4.

0d636c4

kumar303 suggested changes Dec 22, 2016

View reviewed changes

rpl added 2 commits December 22, 2016 17:44

test: check last commit message on single commit branches.

4e27c67

fix: fixed eslint errors in grunt tasks travis-pr-title-lint.

2e95f3d

rpl self-assigned this Dec 22, 2016

kumar303 suggested changes Dec 23, 2016

View reviewed changes

rpl force-pushed the test/changelog-lint-pullrequest-title branch from fc0ad41 to e1d06d5 Compare December 24, 2016 04:27

rpl changed the title ~~Run changelog-lint on the pull request title on travis build.~~ test: run changelog-lint on the pull request title on travis build. Dec 24, 2016

rpl changed the title ~~test: run changelog-lint on the pull request title on travis build.~~ test: run changelog-lint on the pull request title on travis build Dec 24, 2016

rpl changed the title ~~test: run changelog-lint on the pull request title on travis build~~ Run changelog-lint on the pull request title on travis build. Dec 24, 2016

fix: remove template strings and fix weird editor auto-indentation

acbc022

rpl force-pushed the test/changelog-lint-pullrequest-title branch from e1d06d5 to acbc022 Compare December 24, 2016 05:04

fix: remove git-hooks from the npm dev dependencies

9c7c0fc

rpl changed the title ~~Run changelog-lint on the pull request title on travis build.~~ test: run changelog-lint on the pull request title on travis build Dec 24, 2016

docs: Updated CONTRIBUTING.md

cac5926

rpl changed the title ~~test: run changelog-lint on the pull request title on travis build~~ test: Changed changelog linting to check the pull request title during travis builds Dec 26, 2016

rpl changed the title ~~test: Changed changelog linting to check the pull request title during travis builds~~ test: Changed changelog linting to check the pull request title Dec 26, 2016

rpl mentioned this pull request Dec 26, 2016

test: Changed travis config to run tests with both npm and yarn #710

Closed

2 tasks

kumar303 approved these changes Dec 27, 2016

View reviewed changes

test: Updated the message in the installed git-hook

63b55a5

rpl merged commit 5558758 into mozilla:master Dec 28, 2016

muffinresearch removed the state: pull request ready label Dec 28, 2016

test: Changed changelog linting to check the pull request title #704

test: Changed changelog linting to check the pull request title #704

Conversation

rpl commented Dec 22, 2016

rpl commented Dec 22, 2016

coveralls commented Dec 22, 2016

rpl commented Dec 22, 2016

coveralls commented Dec 22, 2016 • edited Loading

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kumar303 commented Dec 22, 2016

coveralls commented Dec 22, 2016 • edited Loading

rpl commented Dec 22, 2016

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl Dec 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 23, 2016 • edited Loading

rpl commented Dec 23, 2016

coveralls commented Dec 24, 2016 • edited Loading

coveralls commented Dec 24, 2016 • edited Loading

coveralls commented Dec 24, 2016 • edited Loading

rpl commented Dec 24, 2016

coveralls commented Dec 26, 2016 • edited Loading

rpl commented Dec 26, 2016

kumar303 left a comment

Choose a reason for hiding this comment

coveralls commented Dec 28, 2016 • edited Loading

rpl commented Dec 28, 2016

coveralls commented Dec 22, 2016 •

edited

Loading

coveralls commented Dec 22, 2016 •

edited

Loading

rpl Dec 23, 2016 •

edited

Loading

coveralls commented Dec 23, 2016 •

edited

Loading

coveralls commented Dec 24, 2016 •

edited

Loading

coveralls commented Dec 24, 2016 •

edited

Loading

coveralls commented Dec 24, 2016 •

edited

Loading

coveralls commented Dec 26, 2016 •

edited

Loading

coveralls commented Dec 28, 2016 •

edited

Loading