Improve error handling (#36)

* better error handling previously we did not do a good job of handling errors that occur when connecting to chrome to capture the heap snapshot Also changes the config.error flag to config.failTests for added clarity about what the flag does * add test coverage for ignored classes
steveszc · Mar 12, 2021 · 7c9fbbe · 7c9fbbe
1 parent d013185
commit 7c9fbbe
Show file tree

Hide file tree

Showing 10 changed files with 121 additions and 46 deletions.
diff --git a/README.md b/README.md
@@ -103,7 +103,7 @@ Configuration
 
 'ember-cli-memory-leak-detector': {
   enabled: process.env.DETECT_MEMORY_LEAKS || false,
-  error: false,
+  failTests: false,
   remoteDebuggingPort: '9222',
   ignoreClasses: ['ExpectedLeakyClass'],
   writeSnapshot: true
@@ -112,10 +112,10 @@ Configuration
 ```
 
 1. `enabled` (default `true`)
-Set to false to disables memory leak detection. 
+Set to false to disables memory leak detection. Consider using an environment variable (`process.env.DETECT_MEMORY_LEAKS`) to control when memory leak detection is run.
 
-2. `error`: (default `true`)
-By default, an error is thrown when a leak is detected, causing test failure. Set this to false to prevent memory leaks from causing your tests to fail, and instead log a console warning.
+2. `failTests`: (default `true`)
+By default when a leak is detected we add a failed test. Set this to `false` to prevent memory leaks from causing your tests to fail, and instead log a console warning.
 
 3. `remoteDebuggingPort`: (default `9222`)
 Configures which port to connect to the testem Chrome instance. This value must match the `--remote-debugging-port` flag set in your app's `testem.js`

diff --git a/config/environment.js b/config/environment.js
@@ -4,7 +4,7 @@ module.exports = function (/* environment, appConfig */) {
   return {
     "ember-cli-memory-leak-detector": {
       enabled: true,
-      error: true,
+      failTests: true,
       remoteDebuggingPort: 9222,
       ignoreClasses: [],
       writeSnapshot: false,

diff --git a/lib/attach-middleware.js b/lib/attach-middleware.js
@@ -18,17 +18,17 @@ function handleDetectMemoryLinkRequest(addon) {
 
       const retainedClasses = results.filter(([name]) => name !== 'App');
 
-      const errorClasses = retainedClasses.filter(
+      const leakedClasses = retainedClasses.filter(
         ([name]) => !config.ignoreClasses.includes(name)
       );
-      const warningClasses = retainedClasses.filter(([name]) =>
+      const ignoredLeakedClasses = retainedClasses.filter(([name]) =>
         config.ignoreClasses.includes(name)
       );
 
-      res.json({ error: config.error, errorClasses, warningClasses });
+      res.json({ failWithLeakedClasses: config.failTests, leakedClasses, ignoredLeakedClasses });
     } catch (error) {
       addon.ui.writeError({ message: `${addon.name}: ${error}` });
-      res.json({});
+      res.json({ error: `${error}` });
     }
   };
 }
diff --git a/lib/detect-memory-leak.js b/lib/detect-memory-leak.js
@@ -31,7 +31,10 @@ async function createCDPClient(title, port) {
     const client = await CDP({ target: testTarget, port });
     return client;
   } catch (e) {
-    throw new Error(`Unable to connect to chrome. ${e}`);
+    if (e.message.includes('ECONNREFUSED')) {
+        e.message += '\nMake sure to configure --remote-debugging-port in testem.js'
+      }
+    throw `Unable to connect to chrome. ${e}`;
   }
 }
 

diff --git a/lib/templates/test-body-footer.html b/lib/templates/test-body-footer.html
@@ -1,5 +1,16 @@
 <script>
   (function () {
+    const DETECTION_IS_NOT_POSSIBLE = 'Memory leak detection is currently available only in the Google Chrome browser via Testem';
+    const LEAKED_CLASSES = 'The following classes were retained in the heap: \n';
+    const LEAKED_IGNORED_CLASSES = 'The following ignored classes were retained in the heap: \n';
+    const MODULE_NAME = 'ember-cli-memory-leak-detector';
+    const TEST_NAME = 'detect memory leaks';
+    const NO_LEAK = 'Is not retained in the heap';
+    const LEAK = 'Is retained in the heap';
+    const DETECTION = 'memory leak detected';
+    const NO_DETECTION = 'No memory leaks detected';
+    const IGNORED = '(ignored)';
+
     var canCaptureHeapSnapshot =
         window.chrome !== null
         && window.navigator.vendor === 'Google Inc.'
@@ -15,7 +26,7 @@
         Testem.afterTests(detectMemoryLeaksWithTestem);
       }
     } else {
-      console.warn('Memory leak detection is currently available only in the Google Chrome browser via Testem');
+      console.warn(DETECTION_IS_NOT_POSSIBLE);
     }
 
     async function detectMemoryLeaksWithTestem(_, _, callback) {
@@ -25,25 +36,32 @@
       let json = await response.json();
       let endTime = Date.now();
 
-      let hasErrors = json.errorClasses.length;
-      let hasWarnings = json.warningClasses.length;
+      let error = json.error;
+      let shouldFail = json.failWithLeakedClasses;
+      let hasLeaks = json.leakedClasses && json.leakedClasses.length;
+      let hasIgnoredLeaks = json.ignoredLeakedClasses && json.ignoredLeakedClasses.length;
+
+      if (hasLeaks) {
+        console[shouldFail ? 'error' : 'warn'](LEAKED_CLASSES + json.leakedClasses.map(([name, count]) => `${name} ${count}x`).join('\n') + '\n');
+      }
 
-      if (hasErrors) {
-        console[json.error ? 'error' : 'warn']('The following classes were detected in the heap snapshot: \n' + json.errorClasses.map(([name, count]) => `${name} ${count}x`).join('\n') + '\n');
+      if (hasIgnoredLeaks) {
+        console.warn(LEAKED_IGNORED_CLASSES + json.ignoredLeakedClasses.map(([name, count]) => `${name} ${count}x ${IGNORED}`).join('\n') + '\n');
       }
-      if (hasWarnings) {
-        console.warn('The following ignored classes were detected in the heap snapshot: \n' + json.warningClasses.map(([name, count]) => `${name} ${count}x`).join('\n') + '\n');
+
+      if (error) {
+        console.error(error);
       }
 
       Testem.emit('test-result', {
-        name: `ember-cli-memory-leak-detector: detect memory leaks`,
+        name: `${MODULE_NAME}: ${TEST_NAME}`,
         total: 1,
-        passed: json.error && !hasErrors ? 0 : 1,
-        failed: json.error && hasErrors ? 1 : 0,
-        skipped: !json.error ? 1 : 0,
+        passed: !error && shouldFail && !hasLeaks ? 0 : 1,
+        failed: error || shouldFail && hasLeaks ? 1 : 0,
+        skipped: error || shouldFail ? 0 : 1,
         todo: 0,
         runDuration: endTime - startTime
-      })
+      });
 
       if (callback) {
         callback();
@@ -52,38 +70,46 @@
 
     async function detectMemoryLeaksWithQUnit(_, _, callback) {
       await new Promise((resolve) => {
-        QUnit.module('ember-cli-memory-leak-detector', {
+        QUnit.module(MODULE_NAME, {
           before: [],
           beforeEach: [],
           afterEach: [],
           after: []
         });
 
-        QUnit.test('detect memory leaks', Object.assign(async function(assert) {
+        QUnit.test(TEST_NAME, Object.assign(async function(assert) {
           let title = encodeURI(document.title);
           let response = await fetch(`/detect_memory_leak?title=${title}`);
           let json = await response.json();
-          let hasErrors = json.errorClasses.length;
-          let hasWarnings = json.warningClasses.length;
 
-          if (hasErrors) {
-            console[json.error ? 'error' : 'warn']('The following classes were detected in the heap snapshot: \n' + json.errorClasses.map(([name, count]) => `${name} ${count}x`).join('\n') + '\n');
+          let error = json.error;
+          let shouldFail = json.failWithLeakedClasses;
+          let hasLeaks = json.leakedClasses && json.leakedClasses.length;
+          let hasIgnoredLeaks = json.ignoredLeakedClasses && json.ignoredLeakedClasses.length;
+
+          if (error) {
+            throw new Error(error);
+          }
+
+          if (hasLeaks) {
+            console[shouldFail ? 'error' : 'warn'](LEAKED_CLASSES + json.leakedClasses.map(([name, count]) => `${name} ${count}x`).join('\n') + '\n');
 
-            for (const error of json.errorClasses) {
-              assert.equal(error[1], json.error ? 0 : error[1], `memory leak detected: ${error[0]} ${json.error ? '' : '(ignored)'}`)
+            for (const error of json.leakedClasses) {
+              assert.false(shouldFail ? true : false, `${DETECTION}: ${error[0]} ${shouldFail ? '' : IGNORED}`);
             }
           }
 
-          if (hasWarnings) {
-            console.warn('The following ignored classes were detected in the heap snapshot: \n' + json.warningClasses.map(([name, count]) => `${name} ${count}x`).join('\n') + '\n');
+          if (hasIgnoredLeaks) {
+            console.warn(LEAKED_IGNORED_CLASSES + json.ignoredLeakedClasses.map(([name, count]) => `${name} ${count}x ${IGNORED}`).join('\n') + '\n');
 
-            for (const warning of json.warningClasses) {
-              assert.equal(warning[1], warning[1], `${warning[0]} x${warning[1]} (ignored)`)
+            for (const warning of json.ignoredLeakedClasses) {
+              assert.false(false, `${warning[0]} ${warning[1]}x ${IGNORED}`);
             }
           }
 
-          if (!hasErrors && !hasWarnings) {
-            assert.ok(true, 'No memory leaks detected');
+          if (!hasLeaks && !hasIgnoredLeaks) {
+            console.log(NO_DETECTION);
+            assert.false(false, NO_DETECTION);
           }
 
           resolve();

diff --git a/node-tests/acceptance/memory-leak-detector-test.js b/node-tests/acceptance/memory-leak-detector-test.js
@@ -12,10 +12,9 @@ describe("Acceptance | Memory leak detection", function () {
     } catch ({ exitCode, stdout }) {
       assert.strictEqual(exitCode, 1, "Exits with non-zero status code");
       assert(stdout.includes("LeakyService"), "Reports the leaked service");
-      assert(
-        !stdout.includes("NonleakyService"),
-        "Only reports the leaked service"
-      );
+      assert(stdout.includes('LeakyComponent 1x (ignored)'), "Warns about the ignored LeakyComponent")
+      assert(!stdout.includes("NonleakyService"), "Only reports the leaked service");
+      assert(stdout.includes('ember-cli-memory-leak-detector: detect memory leaks'), 'ran memory leak detection');
     }
   });
 
@@ -25,15 +24,22 @@ describe("Acceptance | Memory leak detection", function () {
     } catch ({ exitCode, stdout }) {
       assert.strictEqual(exitCode, 1, "Exits with non-zero status code");
       assert(stdout.includes("LeakyService"), "Reports the leaked service");
-      assert(
-        !stdout.includes("NonleakyService"),
-        "Only reports the leaked service"
-      );
+      assert(stdout.includes('LeakyComponent 1x (ignored)'), "Warns about the ignored LeakyComponent")
+      assert(!stdout.includes("NonleakyService"), "Only reports the leaked service");
+      assert(stdout.includes('ember-cli-memory-leak-detector: detect memory leaks'), 'ran memory leak detection');
     }
   });
 
+  it("passes tests if only ignored classes are detected", async function () {
+    let { exitCode, stdout } = await execa("ember", ["test", '--filter=leaky component']);
+    assert.strictEqual(exitCode, 0, "Exits with a zero status code");
+    assert(stdout.includes('LeakyComponent 1x (ignored)'), "Warns about the ignored LeakyComponent");
+    assert(stdout.includes('ember-cli-memory-leak-detector: detect memory leaks'), 'ran memory leak detection');
+  });
+
   it("passes tests if no memory leaks are detected", async function () {
-    let { exitCode } = await execa("ember", ["test", "--filter=nonleaky"]);
+    let { exitCode, stdout } = await execa("ember", ["test", "--filter=nonleaky"]);
     assert.strictEqual(exitCode, 0, "Exits with a zero status code");
+    assert(stdout.includes('ember-cli-memory-leak-detector: detect memory leaks'), 'ran memory leak detection');
   });
 });
diff --git a/tests/dummy/app/components/leaky-component.hbs b/tests/dummy/app/components/leaky-component.hbs
@@ -0,0 +1,2 @@
+<button type="button"></button>
+total clicks anywhere: <span id="clicks">{{this.totalClicks}}</span>
diff --git a/tests/dummy/app/components/leaky-component.js b/tests/dummy/app/components/leaky-component.js
@@ -0,0 +1,16 @@
+import Component from '@glimmer/component';
+import { tracked } from '@glimmer/tracking';
+import { action } from '@ember/object';
+
+export default class LeakyComponent extends Component {
+  @tracked totalClicks = 0;
+
+  constructor() {
+    super(...arguments);
+    window.addEventListener('click', this.handleClick);
+  }
+
+  @action handleClick() {
+    this.totalClicks++;
+  }
+}
diff --git a/tests/dummy/config/environment.js b/tests/dummy/config/environment.js
@@ -20,7 +20,11 @@ module.exports = function(environment) {
     APP: {
       // Here you can pass flags/options to your application instance
       // when it is created
-    }
+    },
+
+    "ember-cli-memory-leak-detector": {
+      ignoreClasses: ["LeakyComponent"],
+    },
   };
 
   if (environment === 'development') {

diff --git a/tests/integration/leaky-component-test.js b/tests/integration/leaky-component-test.js
@@ -0,0 +1,18 @@
+import { module, test } from 'qunit';
+import { setupRenderingTest } from 'ember-qunit';
+import { click, render } from '@ember/test-helpers';
+import { hbs } from 'ember-cli-htmlbars';
+
+module('Integration | Component | leaky component', function(hooks) {
+  setupRenderingTest(hooks);
+
+  test('tracks window clicks', async function(assert) {
+    await render(hbs`<LeakyComponent />`);
+
+    assert.dom('#clicks').hasText('0');
+
+    await click('button');
+
+    assert.dom('#clicks').hasText('1');
+  });
+});