From a6768c512552ba21621eb3b3b33727c973b1643c Mon Sep 17 00:00:00 2001 From: Alexander Afanasyev Date: Thu, 24 Nov 2016 13:00:40 -0500 Subject: [PATCH] feat(no-expect-in-setup-teardown): add no-expect-in-setup-teardown rule --- README.md | 28 +++--- docs/rules/no-expect-in-setup-teardown.md | 48 +++++++++ index.js | 6 +- lib/rules/no-expect-in-setup-teardown.js | 50 ++++++++++ test/rules/no-expect-in-setup-teardown.js | 113 ++++++++++++++++++++++ 5 files changed, 230 insertions(+), 15 deletions(-) create mode 100644 docs/rules/no-expect-in-setup-teardown.md create mode 100644 lib/rules/no-expect-in-setup-teardown.js create mode 100644 test/rules/no-expect-in-setup-teardown.js diff --git a/README.md b/README.md index ae23f9c..95da6a0 100644 --- a/README.md +++ b/README.md @@ -64,19 +64,20 @@ configuration files. ### Rules -Rule | Recommended | Options ----- | ----------- | ------- -[named-spy][] | 0 | -[no-focused-tests][] | 2 | -[no-disabled-tests][] | 1 | -[no-suite-dupes][] | 1, `'block'` | `['block', 'branch']` -[no-spec-dupes][] | 1, `'block'` | `['block', 'branch']` -[missing-expect][] | 0, `'expect()'` | expectation function names -[no-suite-callback-args][] | 2 | -[valid-expect][] | 1 | -[no-assign-spyon][] | 0 | -[no-unsafe-spy][] | 1 | -[no-global-setup][] | 2 | +Rule | Recommended | Options +---- | ----------- | ------- +[named-spy][] | 0 | +[no-focused-tests][] | 2 | +[no-disabled-tests][] | 1 | +[no-suite-dupes][] | 1, `'block'` | `['block', 'branch']` +[no-spec-dupes][] | 1, `'block'` | `['block', 'branch']` +[missing-expect][] | 0, `'expect()'` | expectation function names +[no-suite-callback-args][] | 2 | +[valid-expect][] | 1 | +[no-assign-spyon][] | 0 | +[no-unsafe-spy][] | 1 | +[no-global-setup][] | 2 | +[no-expect-in-setup-teardown][] | 0, `'expect()'` | expectation function names For example, using the recommended configuration, the `no-focused-tests` rule is enabled and will cause ESLint to throw an error (with an exit code of `1`) @@ -110,6 +111,7 @@ See [configuring rules][] for more information. [no-assign-spyon]: docs/rules/no-assign-spyon.md [no-unsafe-spy]: docs/rules/no-unsafe-spy.md [no-global-setup]: docs/rules/no-global-setup.md +[no-expect-in-setup-teardown]: docs/rules/no-expect-in-setup-teardown.md [configuring rules]: http://eslint.org/docs/user-guide/configuring#configuring-rules diff --git a/docs/rules/no-expect-in-setup-teardown.md b/docs/rules/no-expect-in-setup-teardown.md new file mode 100644 index 0000000..a6530f6 --- /dev/null +++ b/docs/rules/no-expect-in-setup-teardown.md @@ -0,0 +1,48 @@ +# Discourage making expectations in setup and teardown functions (no-expect-in-setup-teardown) + +Making expectations inside test setup or teardown functions is sometimes a sign of a bad test design. + +It might negatively impact readability as a reader might think you have the actual test logic splattered between setup, teardown and the test functions themselves. + +Sometimes there is actually a need to test the result of a setup/teardown function call. In cases like these, it is recommended to create a separate test to test the setup/teardown process. + +## Rule details + +This rule triggers a warning if there is an `expect()` call inside `beforeEach()`, `afterEach()`, `beforeAll()` and `afterAll()`. + +An array of expect function names may be passed to the configuration of this rule. By default only `expect()` is used. + +The following patterns are considered warnings: + +```js +beforeEach(function() { expect(true).toBe(true); }); +afterEach(function() { expect(true).toBe(true); }); +beforeAll(function() { expect(true).toBe(true); }); +afterAll(function() { expect(true).toBe(true); }); +``` + +The following patterns are not warnings: + +```js +beforeEach(function() { someOtherFunction(); }); +afterEach(function() {}); +beforeAll(function() { someOtherFunction(); }); +afterAll(function() {}); +``` + +### AngularJS `$httpBackend` rule configuration example + +```yaml +rules: + no-expect-in-setup-teardown: + - 2 + - expect() + - $httpBackend.expect() + - $httpBackend.expectDELETE() + - $httpBackend.expectGET() + - $httpBackend.expectJSONP() + - $httpBackend.expectHEAD() + - $httpBackend.expectPATCH() + - $httpBackend.expectPOST() + - $httpBackend.expectPUT() +``` diff --git a/index.js b/index.js index 4247770..a24cd26 100644 --- a/index.js +++ b/index.js @@ -12,7 +12,8 @@ module.exports = { 'valid-expect': require('./lib/rules/valid-expect'), 'no-assign-spyon': require('./lib/rules/no-assign-spyon'), 'no-unsafe-spy': require('./lib/rules/no-unsafe-spy'), - 'no-global-setup': require('./lib/rules/no-global-setup') + 'no-global-setup': require('./lib/rules/no-global-setup'), + 'no-expect-in-setup-teardown': require('./lib/rules/no-expect-in-setup-teardown') }, configs: { recommended: { @@ -27,7 +28,8 @@ module.exports = { 'jasmine/valid-expect': 1, 'jasmine/no-assign-spyon': 0, 'jasmine/no-unsafe-spy': 1, - 'jasmine/no-global-setup': 2 + 'jasmine/no-global-setup': 2, + 'jasmine/no-expect-in-setup-teardown': 0 } } } diff --git a/lib/rules/no-expect-in-setup-teardown.js b/lib/rules/no-expect-in-setup-teardown.js new file mode 100644 index 0000000..4fbc914 --- /dev/null +++ b/lib/rules/no-expect-in-setup-teardown.js @@ -0,0 +1,50 @@ +'use strict' + +/** + * @fileoverview Discourage having expect in setup and teardown functions + * @author Alexander Afanasyev + */ + +module.exports = function (context) { + var allowed = context.options.length ? context.options : ['expect()'] + var setupRegexp = /^(before|after)(Each|All)$/ + + var insideSetup = false + var setupFunctionName + + function buildName (node) { + if (node.type === 'CallExpression') { + return buildName(node.callee) + '()' + } + if (node.type === 'MemberExpression') { + return buildName(node.object) + '.' + node.property.name + } + if (node.type === 'Identifier') { + return node.name + } + } + + return { + CallExpression: function (node) { + if (setupRegexp.test(node.callee.name)) { + insideSetup = true + setupFunctionName = node.callee.name + return + } + + if (insideSetup) { + var functionName = buildName(node) + if (allowed.indexOf(functionName) > -1) { + context.report(node, 'Unexpected "' + functionName + '" call in "' + setupFunctionName + '()"') + } + } + }, + + 'CallExpression:exit': function (node) { + if (setupRegexp.test(node.callee.name)) { + insideSetup = false + setupFunctionName = undefined + } + } + } +} diff --git a/test/rules/no-expect-in-setup-teardown.js b/test/rules/no-expect-in-setup-teardown.js new file mode 100644 index 0000000..6a1c9a6 --- /dev/null +++ b/test/rules/no-expect-in-setup-teardown.js @@ -0,0 +1,113 @@ +'use strict' + +var rule = require('../../lib/rules/no-expect-in-setup-teardown') +var RuleTester = require('eslint').RuleTester + +var eslintTester = new RuleTester() + +eslintTester.run('no-expect-in-setup-teardown', rule, { + valid: [ + 'beforeEach(function() {});', + 'afterEach(function() {});', + 'beforeAll(function() {});', + 'afterAll(function() {});', + 'it("", function() { expect(true).toBe(true); })', + 'beforeEach(function() { someOtherFunction(); });', + 'afterEach(function() { someOtherFunction(); });', + 'beforeAll(function() { someOtherFunction(); });', + 'afterAll(function() { someOtherFunction(); });', + 'expect(true).toBe(true);', + { + code: 'it("", function() {$httpBackend.expectGET();})', + options: [ + '$httpBackend.expectGET()' + ] + }, + { + code: 'it("", function() {return $httpBackend.expectGET();})', + options: [ + '$httpBackend.expectGET()' + ] + }, + { + code: 'it("", function() {a.deeply.nested().expect.expression();})', + options: [ + 'a.deeply.nested().expect.expression()' + ] + }, + { + code: 'it("", function() { expect(true).toBe(true); })', + options: [ + 'a.deeply.nested().expect.expression()' + ] + } + ], + + invalid: [ + { + code: 'beforeEach(function() { expect(true).toBe(true); });', + errors: [ + { + message: 'Unexpected "expect()" call in "beforeEach()"' + } + ] + }, + { + code: 'afterEach(function() { expect(true).toBe(true); });', + errors: [ + { + message: 'Unexpected "expect()" call in "afterEach()"' + } + ] + }, + { + code: 'beforeAll(function() { expect(true).toBe(true); });', + errors: [ + { + message: 'Unexpected "expect()" call in "beforeAll()"' + } + ] + }, + { + code: 'afterAll(function() { expect(true).toBe(true); });', + errors: [ + { + message: 'Unexpected "expect()" call in "afterAll()"' + } + ] + }, + { + code: 'beforeEach(function() {$httpBackend.expectGET();})', + options: [ + '$httpBackend.expectGET()' + ], + errors: [ + { + message: 'Unexpected "$httpBackend.expectGET()" call in "beforeEach()"' + } + ] + }, + { + code: 'beforeAll(function() {return $httpBackend.expectGET();})', + options: [ + '$httpBackend.expectGET()' + ], + errors: [ + { + message: 'Unexpected "$httpBackend.expectGET()" call in "beforeAll()"' + } + ] + }, + { + code: 'afterEach(function() {a.deeply.nested().expect.expression()})', + options: [ + 'a.deeply.nested().expect.expression()' + ], + errors: [ + { + message: 'Unexpected "a.deeply.nested().expect.expression()" call in "afterEach()"' + } + ] + } + ] +})