Repository: ember-cli/eslint-plugin-ember (js)

Issue:
Side effects in conditions are not detected
The next code throws a no-side-effects error:

```
foo: computed(function() {
  this.set('bar', 'someValue');
})
```

Yet this code doesn't:

```
foo: computed(function() {
  const condition = true;
  if (condition) {
    this.set('bar', 'someValue');
  }
})
```

Pull request description:
Updating no-side-effects rule to better detect sets inside of blocks
Fixes #259.

Patch:
diff --git a/docs/rules/no-empty-attrs.md b/docs/rules/no-empty-attrs.md
index 2b7e9ed3..54a1cdd8 100644
--- a/docs/rules/no-empty-attrs.md
+++ b/docs/rules/no-empty-attrs.md
@@ -2,7 +2,7 @@
 
 ### Rule name: `no-empty-attrs`
 
-Ember Data could handle lack of specified types in model description. Nonetheless this could lead to ambiguity. Therefore always supply proper attribute type to ensure the right data transform is used.
+Ember Data handles not specifying a transform in model description. Nonetheless this could lead to ambiguity. This rule ensures that the right transform is specified for every attribute.
 
 ```javascript
 const { Model, attr } = DS;
diff --git a/lib/rules/no-on-calls-in-components.js b/lib/rules/no-on-calls-in-components.js
index a2be6b95..83304c96 100644
--- a/lib/rules/no-on-calls-in-components.js
+++ b/lib/rules/no-on-calls-in-components.js
@@ -4,13 +4,13 @@ const ember = require('../utils/ember');
 const utils = require('../utils/utils');
 
 //----------------------------------------------
-// General rule - Don\'t use .on() in components
+// Don’t use .on() for component lifecycle events.
 //----------------------------------------------
 
 module.exports = {
   meta: {
     docs: {
-      description: 'Prevents usage of `on` calls in components',
+      description: 'Prevents usage of `on` to call lifecycle hooks in components',
       category: 'Best Practices',
       recommended: true,
       url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-on-calls-in-components.md'
@@ -19,15 +19,29 @@ module.exports = {
   },
 
   create(context) {
-    const message = 'Don\'t use .on() in components';
+    const message = 'Don\'t use .on() for component lifecycle events.';
+    const lifecycleHooks = ['init',
+      'didUpdateAttrs',
+      'didReceiveAttrs',
+      'willUpdate',
+      'willRender',
+      'didInsertElement',
+      'didUpdate',
+      'didRender',
+      'willDestroyElement',
+      'willClearRender',
+      'didDestroyElement'
+    ];
 
     const filePath = context.getFilename();
 
     const isOnCall = function (node) {
       const value = node.value;
       const callee = value.callee;
+      const args = utils.parseArgs(value);
 
       return utils.isCallExpression(value) &&
+        lifecycleHooks.indexOf(args[0]) > -1 &&
         (
           (utils.isIdentifier(callee) && callee.name === 'on') ||
           (
diff --git a/lib/rules/no-side-effects.js b/lib/rules/no-side-effects.js
index ca89207b..dde6d375 100644
--- a/lib/rules/no-side-effects.js
+++ b/lib/rules/no-side-effects.js
@@ -1,6 +1,7 @@
 'use strict';
 
 const utils = require('../utils/utils');
+const ember = require('../utils/ember');
 
 //------------------------------------------------------------------------------
 // General rule - Don't introduce side-effects in computed properties
@@ -18,6 +19,8 @@ module.exports = {
   },
 
   create(context) {
+    let emberImportAliasName;
+
     const message = 'Don\'t introduce side-effects in computed properties';
 
     const report = function (node) {
@@ -25,35 +28,39 @@ module.exports = {
     };
 
     return {
+      ImportDeclaration(node) {
+        if (!emberImportAliasName) {
+          emberImportAliasName = ember.getEmberImportAliasName(node);
+        }
+      },
+
       CallExpression(node) {
         const callee = node.callee;
-        const fnNodes = utils.findNodes(node.arguments, 'FunctionExpression');
-
-        if (callee && callee.name === 'computed' && fnNodes) {
-          fnNodes.forEach((fnNode) => {
-            const fnBody = fnNode.body ? fnNode.body.body : fnNode.body;
-            const fnExpressions = utils.findNodes(fnBody, 'ExpressionStatement');
-
-            fnExpressions.forEach((fnExpression) => {
-              const fnCallee = fnExpression.expression.callee;
-
-              if (
-                utils.isIdentifier(fnCallee) &&
-                fnCallee.name === 'set') {
-                report(fnExpression);
-                return;
-              }
-
-              if (
-                utils.isMemberExpression(fnCallee) &&
-                utils.isThisExpression(fnCallee.object) &&
-                utils.isIdentifier(fnCallee.property) &&
-                fnCallee.property.name === 'set'
-              ) {
-                report(fnExpression);
-              }
-            });
-          });
+        const isEmberSet = (
+          utils.isIdentifier(callee) &&
+          callee.name === 'set'
+        ) || (
+          utils.isMemberExpression(callee) &&
+          (utils.isThisExpression(callee.object) || callee.object.name === 'Ember' || callee.object.name === emberImportAliasName) &&
+          utils.isIdentifier(callee.property) &&
+          callee.property.name === 'set'
+        );
+
+        if (isEmberSet) {
+          const ancestors = context.getAncestors();
+          const computedIndex = ancestors.findIndex(ember.isComputedProp);
+          const setPropertyFunctionIndex = ancestors.findIndex(ancestor => (
+            ancestor.type === 'Property' &&
+            ancestor.key.name === 'set' &&
+            utils.isFunctionExpression(ancestor.value)
+          ));
+
+          if (
+            computedIndex > -1 &&
+            (setPropertyFunctionIndex === -1 || setPropertyFunctionIndex < computedIndex)
+          ) {
+            report(node);
+          }
         }
       },
     };


Test patch:
diff --git a/docs/rules/no-test-import-export.md b/docs/rules/no-test-import-export.md
new file mode 100644
index 00000000..e19a2bba
--- /dev/null
+++ b/docs/rules/no-test-import-export.md
@@ -0,0 +1,76 @@
+# No importing of test files
+
+**TL;DR** Do not import from a test file (a file ending in "-test.js") in another test file. Doing so will cause the module and tests from the imported file to be executed again. Similarly, test files should not have any exports.
+
+Due to how qunit unloads a test module, importing a test file will cause any modules and tests within the file to be executed every time it gets loaded. If you want to import any helper functions or tests to be shared between multiple test files, please make it a test-helper that gets imported by the test file.
+
+```javascript
+// GOOD
+
+import setupModule from './some-test-helper';
+import { module, test } from 'qunit';
+
+module('Acceptance | module', setupModule());
+```
+
+```javascript
+// GOOD
+
+// some-test-helper.js
+export function beforeEachSetup(){ ... };
+```
+
+```javascript
+// GOOD
+
+// some-test-helper.js
+function beforeEachSetup(){};
+
+export default {beforeEachSetup};
+```
+
+```javascript
+// BAD
+
+import setupModule from './some-other-test';
+import { module, test } from 'qunit';
+
+module('Acceptance | module', setupModule());
+```
+
+```javascript
+// BAD
+
+import {
+  beforeEachSetup,
+  testMethod,
+} from './some-other-test';
+import { module, test } from 'qunit';
+
+module('Acceptance | module', beforeEachSetup());
+```
+
+```javascript
+// BAD
+
+import testModule from '../../test-dir/another-test';
+import { module, test } from 'qunit';
+
+module('Acceptance | module', testModule());
+```
+
+```javascript
+// BAD
+
+// some-test.js
+export function beforeEachSetup(){};
+```
+
+```javascript
+// BAD
+
+// some-test.js
+function beforeEachSetup(){};
+
+export default {beforeEachSetup};
+```
diff --git a/lib/rules/no-test-import-export.js b/lib/rules/no-test-import-export.js
new file mode 100644
index 00000000..3be5bac6
--- /dev/null
+++ b/lib/rules/no-test-import-export.js
@@ -0,0 +1,57 @@
+/**
+ * @fileOverview no importing of test files and no exporting in test files
+ */
+
+'use strict';
+
+const NO_IMPORT_MESSAGE
+  = 'Do not import from a test file (a file ending in "-test.js") in another test file. Doing so will cause the module and tests from the imported file to be executed again.';
+
+const NO_EXPORT_MESSAGE
+  = 'No exports from test file. Any exports should be done in a test helper.';
+
+module.exports = {
+  docs: {
+    description:
+      'Disallow importing of "-test.js" in a test file and exporting from a test file.',
+    category: 'Testing',
+    recommended: false
+  },
+  meta: {
+    importMessage: NO_IMPORT_MESSAGE,
+    exportMessage: NO_EXPORT_MESSAGE
+  },
+
+  create: function create(context) {
+    const noExports = function (node) {
+      const fileName = context.getFilename();
+      if (!fileName.endsWith('-test.js')) {
+        return;
+      }
+
+      context.report({
+        message: NO_EXPORT_MESSAGE,
+        node
+      });
+    };
+
+    return {
+      ImportDeclaration(node) {
+        const importSource = node.source.value;
+
+        if (importSource.endsWith('-test')) {
+          context.report({
+            message: NO_IMPORT_MESSAGE,
+            node
+          });
+        }
+      },
+      ExportNamedDeclaration(node) {
+        noExports(node);
+      },
+      ExportDefaultDeclaration(node) {
+        noExports(node);
+      }
+    };
+  }
+};
diff --git a/tests/lib/rules/no-on-calls-in-components.js b/tests/lib/rules/no-on-calls-in-components.js
index ded94ce4..84650adb 100644
--- a/tests/lib/rules/no-on-calls-in-components.js
+++ b/tests/lib/rules/no-on-calls-in-components.js
@@ -10,7 +10,7 @@ const RuleTester = require('eslint').RuleTester;
 // ------------------------------------------------------------------------------
 
 const eslintTester = new RuleTester();
-const message = 'Don\'t use .on() in components';
+const message = 'Don\'t use .on() for component lifecycle events.';
 
 eslintTester.run('no-on-calls-in-components', rule, {
   valid: [
@@ -70,6 +70,10 @@ eslintTester.run('no-on-calls-in-components', rule, {
       code: 'export default Component.extend({actions: {abc() {$("body").on("click", def).on("click", function () {})}}});',
       parserOptions: { ecmaVersion: 6, sourceType: 'module' },
     },
+    {
+      code: 'export default Component.extend({abc: on("nonLifecycleEvent", function() {})});',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+    },
   ],
   invalid: [
     {
@@ -103,7 +107,7 @@ eslintTester.run('no-on-calls-in-components', rule, {
       code: 'export default CustomComponent.extend({test: on("didInsertElement", function () {})});',
       parserOptions: { ecmaVersion: 6, sourceType: 'module' },
       errors: [{
-        message: 'Don\'t use .on() in components',
+        message: 'Don\'t use .on() for component lifecycle events.',
       }],
     },
     {
@@ -111,7 +115,7 @@ eslintTester.run('no-on-calls-in-components', rule, {
       code: 'export default CustomComponent.extend({test: on("didInsertElement", function () {})});',
       parserOptions: { ecmaVersion: 6, sourceType: 'module' },
       errors: [{
-        message: 'Don\'t use .on() in components',
+        message: 'Don\'t use .on() for component lifecycle events.',
       }],
     },
     {
@@ -119,7 +123,7 @@ eslintTester.run('no-on-calls-in-components', rule, {
       code: 'export default Component.extend({test: on("didInsertElement", function () {})});',
       parserOptions: { ecmaVersion: 6, sourceType: 'module' },
       errors: [{
-        message: 'Don\'t use .on() in components',
+        message: 'Don\'t use .on() for component lifecycle events.',
       }],
     },
   ],
diff --git a/tests/lib/rules/no-side-effects.js b/tests/lib/rules/no-side-effects.js
index 91619a20..c41da6c2 100644
--- a/tests/lib/rules/no-side-effects.js
+++ b/tests/lib/rules/no-side-effects.js
@@ -16,6 +16,18 @@ eslintTester.run('no-side-effects', rule, {
       code: 'testAmount: alias("test.length")',
       parserOptions: { ecmaVersion: 6, sourceType: 'module' },
     },
+    {
+      code: 'testAmount: computed("test.length", { get() { return ""; }, set() { set(this, "testAmount", test.length); }})',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+    },
+    {
+      code: 'let foo = computed("test", function() { someMap.set(this, "testAmount", test.length); return ""; })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+    },
+    {
+      code: 'import Ember from "ember"; import Foo from "some-other-thing"; let foo = computed("test", function() { Foo.set(this, "testAmount", test.length); return ""; });',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' }
+    }
   ],
   invalid: [
     {
@@ -31,5 +43,47 @@ eslintTester.run('no-side-effects', rule, {
         message: 'Don\'t introduce side-effects in computed properties',
       }],
     },
+    {
+      code: 'prop: computed("test", function() {if (get(this, "testAmount")) { set(this, "testAmount", test.length); } return "";})',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'testAmount: computed("test.length", { get() { set(this, "testAmount", test.length); }, set() { set(this, "testAmount", test.length); }})',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'testAmount: computed("test.length", function() { const setSomething = () => { set(this, "testAmount", test.length); }; setSomething(); })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'let foo = computed("test", function() { Ember.set(this, "testAmount", test.length); return ""; })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'import Foo from "ember"; let foo = computed("test", function() { Foo.set(this, "testAmount", test.length); return ""; })',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    },
+    {
+      code: 'import EmberFoo from "ember"; import Foo from "some-other-thing"; let foo = computed("test", function() { EmberFoo.set(this, "testAmount", test.length); return ""; });',
+      parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+      errors: [{
+        message: 'Don\'t introduce side-effects in computed properties'
+      }]
+    }
   ],
 });
diff --git a/tests/lib/rules/no-test-import-export.js b/tests/lib/rules/no-test-import-export.js
new file mode 100644
index 00000000..3da32a7a
--- /dev/null
+++ b/tests/lib/rules/no-test-import-export.js
@@ -0,0 +1,98 @@
+const rule = require('../../../lib/rules/no-test-import-export');
+const RuleTester = require('eslint').RuleTester;
+
+const ruleTester = new RuleTester({
+  parserOptions: {
+    ecmaVersion: 6,
+    sourceType: 'module'
+  }
+});
+const NO_IMPORT_MESSAGE = rule.meta.importMessage;
+const NO_EXPORT_MESSAGE = rule.meta.exportMessage;
+
+ruleTester.run('no-test-file-importing', rule, {
+  valid: [
+    {
+      code: `
+        import setupModule from './some-test-helper';
+        import { module, test } from 'qunit';
+
+        module('Acceptance | module', setupModule());
+      `
+    },
+    {
+      filename: 'tests/some-test-helper.js',
+      code: `
+        export function beforeEachSetup(){};
+      `,
+    }
+  ],
+  invalid: [
+    {
+      code: `
+        import setupModule from './some-other-test';
+        import { module, test } from 'qunit';
+
+        module('Acceptance | module', setupModule());
+      `,
+      errors: [
+        {
+          message: NO_IMPORT_MESSAGE
+        }
+      ]
+    },
+    {
+      code: `
+        import {
+          beforeEachSetup,
+          testName,
+        } from './some-other-test';
+        import { module, test } from 'qunit';
+
+        module('Acceptance | module', beforeEachSetup());
+      `,
+      errors: [
+        {
+          message: NO_IMPORT_MESSAGE
+        }
+      ]
+    },
+    {
+      code: `
+        import testModule from '../../test-dir/another-test';
+        import { module, test } from 'qunit';
+
+        module('Acceptance | module', testModule());
+      `,
+      errors: [
+        {
+          message: NO_IMPORT_MESSAGE
+        }
+      ]
+    },
+    {
+      filename: 'tests/some-test.js',
+      code: `
+        export function beforeEachSetup(){};
+      `,
+      errors: [
+        {
+          message: NO_EXPORT_MESSAGE
+        }
+      ]
+    },
+    {
+      filename: 'tests/some-test.js',
+      code: `
+        function beforeEachSetup(){};
+
+        export default {beforeEachSetup};
+      `,
+      errors: [
+        {
+          message: NO_EXPORT_MESSAGE
+        }
+      ]
+    },
+  ]
+});