From 61be7d50276663add78f0c978596209ca33b897b Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 12 Jan 2016 17:24:52 -0500 Subject: [PATCH 1/5] eslintrc: turn on complexity, max-statements Useful checks: we should use them. --- .eslintrc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.eslintrc b/.eslintrc index 0578e2aed..e3e78971b 100644 --- a/.eslintrc +++ b/.eslintrc @@ -6,8 +6,10 @@ "brace-style": [2, "1tbs", { "allowSingleLine": true }], "comma-dangle": [2, "never"], "comma-style": [2, "first", { exceptions: {ArrayExpression: true, ObjectExpression: true} }], + "complexity": [2, 6], "curly": 2, "eqeqeq": [2, "allow-null"], + "max-statements": [2, 30], "no-shadow-restricted-names": 2, "no-undef": 2, "no-use-before-define": 2, From 52dbe0f6a4991369016e6584a898fd948a66f281 Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 12 Jan 2016 15:51:24 -0500 Subject: [PATCH 2/5] categorize max-statements as Complexity, fix point calc * max-statements rule should go in the Complexity Category * Calculating remediation points for rules in that category is now more complicated, because we have to use regexs to extract the values * Adjusted the overage points here to match RuboCop, since we're trying to standardize around that for these types of checks. --- lib/checks.js | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/lib/checks.js b/lib/checks.js index e278d72a7..40d50561c 100644 --- a/lib/checks.js +++ b/lib/checks.js @@ -14,6 +14,7 @@ var checkCategoryMapping = { "guard-for-in": "Bug Risk", "handle-callback-err": "Bug Risk", "init-declarations": "Clarity", + "max-statements": "Complexity", "no-alert": "Bug Risk", "no-caller": "Compatibility", "no-case-declarations": "Bug Risk", @@ -113,21 +114,44 @@ var categories = function(checkName) { return [checkCategoryMapping[checkName] || "Style"]; }; +// Here Be Dragons: this function extracts the relevant value that triggered the issue for +// checks in the Complexity category. Unfortunately, these values are not available in a +// structured way, so we extract them from strings. That means that any check categorized +// as Complexity MUST have a rule here to extract value. +// +// If this function fails to find a matching string segment, it will return `undefined`. +var messageMetricValue = function(message) { + var match = null; + switch (message.ruleId) { + case "complexity": + match = message.message(/complexity of (\d+)/); + break; + case "max-statements": + match = message.message("/too many statements \((\d+)\)/"); + break; + } + if (null !== match) { + return parseInt(match[1], 10); + } + return null; +}; + var remediationPoints = function(checkName, message) { - if (checkName === "complexity") { + if (categories(checkName) === ["Complexity"]) { // (@base_cost + (overage * @cost_per))*1_000_000 // cost_per: 0.1, base: 1 - var costPer = 100000 + var costPer = 70000 , points = 1000000 , threshold = 10 // TODO: Get this from the eslintrc , overage - , complexity; - - complexity = message.message.match(/complexity of (\d+)/)[1]; - overage = complexity - threshold; + , metricValue; - if (overage > 0) { - points += (costPer * overage); + metricValue = messageMetricValue(message); + if (null !== metricValue) { + overage = metricValue - threshold; + if (overage > 0) { + points += (costPer * overage); + } } return points; From d1a72522295110daf76f3d10e5b82f2a997cf74a Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Tue, 12 Jan 2016 17:50:03 -0500 Subject: [PATCH 3/5] Use configured thresholds for remediation points Pull the configured threshold value for complexity rules from the active config, and use it in calculating remediation points. --- bin/eslint.js | 2 +- lib/checks.js | 16 ++++++++++------ 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/bin/eslint.js b/bin/eslint.js index 31122e6d0..9a2cc4801 100755 --- a/bin/eslint.js +++ b/bin/eslint.js @@ -60,7 +60,7 @@ function buildIssueJson(message, path) { } } }, - remediation_points: checks.remediationPoints(checkName, message) + remediation_points: checks.remediationPoints(checkName, message, cli.getConfigForFile(path)) }; return JSON.stringify(issue); } diff --git a/lib/checks.js b/lib/checks.js index 40d50561c..f7a960686 100644 --- a/lib/checks.js +++ b/lib/checks.js @@ -119,15 +119,15 @@ var categories = function(checkName) { // structured way, so we extract them from strings. That means that any check categorized // as Complexity MUST have a rule here to extract value. // -// If this function fails to find a matching string segment, it will return `undefined`. +// If a matching string segment cannot be found, `null` will be returned. var messageMetricValue = function(message) { var match = null; switch (message.ruleId) { case "complexity": - match = message.message(/complexity of (\d+)/); + match = message.message.match(/complexity of (\d+)/); break; case "max-statements": - match = message.message("/too many statements \((\d+)\)/"); + match = message.message.match(/too many statements \((\d+)\)/); break; } if (null !== match) { @@ -136,13 +136,17 @@ var messageMetricValue = function(message) { return null; }; -var remediationPoints = function(checkName, message) { - if (categories(checkName) === ["Complexity"]) { +var metricThreshold = function(ruleId, eslintConfig) { + return eslintConfig.rules[ruleId][1]; +}; + +var remediationPoints = function(checkName, message, eslintConfig) { + if (categories(checkName)[0] === "Complexity") { // (@base_cost + (overage * @cost_per))*1_000_000 // cost_per: 0.1, base: 1 var costPer = 70000 , points = 1000000 - , threshold = 10 // TODO: Get this from the eslintrc + , threshold = metricThreshold(message.ruleId, eslintConfig) , overage , metricValue; From 64f885a9d6edf777a3d1d721b884ef61299e10e6 Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Wed, 13 Jan 2016 12:29:14 -0500 Subject: [PATCH 4/5] Add tests Setup mocha & write some tests for the checks module. --- .codeclimate.yml | 1 + package.json | 7 +++++++ test/checks_test.js | 49 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 test/checks_test.js diff --git a/.codeclimate.yml b/.codeclimate.yml index 1acff4fbf..a9416a0a5 100644 --- a/.codeclimate.yml +++ b/.codeclimate.yml @@ -12,3 +12,4 @@ ratings: - lib/checks.js" exclude_paths: - "node_modules/**" +- "test/**" diff --git a/package.json b/package.json index 8c91a5fe9..d3adf33a2 100644 --- a/package.json +++ b/package.json @@ -15,5 +15,12 @@ "eslint-plugin-react": "3.6.3", "glob": "5.0.14" }, + "devDependencies": { + "chai": "^2.1.0", + "mocha": "^2.3.4" + }, + "scripts": { + "test": "mocha test" + }, "engine": "node >= 0.12.4" } diff --git a/test/checks_test.js b/test/checks_test.js new file mode 100644 index 000000000..ed086a240 --- /dev/null +++ b/test/checks_test.js @@ -0,0 +1,49 @@ +var checks = require("../lib/checks"); +var expect = require("chai").expect; + +describe("checks module", function() { + describe(".categories()", function() { + it("returns complexity for max-statements", function() { + expect(checks.categories("max-statements")).to.deep.eq(["Complexity"]); + }); + + it("returns style for an unknown check type", function() { + expect(checks.categories("floofle")).to.deep.eq(["Style"]); + }); + }); + + describe(".remediationPoints()", function() { + it("returns the default of 50,000 for a non-complexity issue", function() { + var issue = { ruleId: "eqeqeq", message: "always use ==="}; + expect(checks.remediationPoints(issue.ruleId, issue, null)).to.eq(50000); + }); + + it("calculates the cost for a cyclomatic complexity issue", function() { + var issue = { ruleId: "complexity", message: "Function 'complex' has a complexity of 8." } + , config = eslintConfig({ "complexity": [2, 6] }); + expect(checks.remediationPoints(issue.ruleId, issue, config)).to.eq(1140000); + }); + + it("calculates the cost for a max-statements issue", function() { + var issue = { ruleId: "max-statements", message: "This function has too many statements (38). Maximum allowed is 30." } + , config = eslintConfig({ "max-statements": [2, 30] }); + expect(checks.remediationPoints(issue.ruleId, issue, config)).to.eq(1560000); + }); + + it("uses base complexity cost if metric cannot be found", function() { + var issue = { ruleId: "complexity", message: "has a complexity of \"8\"" } + , config = eslintConfig({ "complexity": [2, 6] }); + expect(checks.remediationPoints(issue.ruleId, issue, config)).to.eq(1000000); + }); + + it("uses base complexity cost if overage is negative somehow", function() { + var issue = { ruleId: "complexity", message: "has a complexity of 8" } + , config = eslintConfig({ "complexity": [2, 10] }); + expect(checks.remediationPoints(issue.ruleId, issue, config)).to.eq(1000000); + }); + + var eslintConfig = function(rulesConfig) { + return { rules: rulesConfig }; + }; + }); +}); From 8f6fcab0797354d48698bd696851a27adc56ee37 Mon Sep 17 00:00:00 2001 From: Will Fleming Date: Wed, 13 Jan 2016 12:29:39 -0500 Subject: [PATCH 5/5] Add `make test` task, use `make` in circle.yml --- Makefile | 5 ++++- circle.yml | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 3b5a0f776..c571bf19e 100644 --- a/Makefile +++ b/Makefile @@ -1,6 +1,9 @@ -.PHONY: image +.PHONY: image test IMAGE_NAME ?= codeclimate/codeclimate-eslint image: docker build --rm -t $(IMAGE_NAME) . + +test: image + docker run $(IMAGE_NAME) npm run test diff --git a/circle.yml b/circle.yml index 9d97e5cda..c5becf697 100644 --- a/circle.yml +++ b/circle.yml @@ -11,7 +11,7 @@ machine: test: override: - - docker build -t=$PRIVATE_REGISTRY/$CIRCLE_PROJECT_REPONAME:b$CIRCLE_BUILD_NUM . + - IMAGE_NAME="$PRIVATE_REGISTRY/$CIRCLE_PROJECT_REPONAME:b$CIRCLE_BUILD_NUM" make test deployment: registry: