fix(spec): re-enable NylasEnv specs and fix duplicate rejection log

Summary:
This re-enables NylasEnv spec and fixes a particularly tricky test
involving the Node event loop.

See the comments I left on process.unhandledRejection and the new spec

Test Plan: Manually run test in isolation and whole suite. All green!

Reviewers: juan, mark, halla, spang

Reviewed By: spang

Differential Revision: https://phab.nylas.com/D3942

spec/nylas-env-spec.es6

@@ -1,6 +1,6 @@
 import { remote } from 'electron';
 
-xdescribe("the `NylasEnv` global", function nylasEnvSpec() {
+describe("the `NylasEnv` global", function nylasEnvSpec() {
   describe('window sizing methods', () => {
     describe('::getPosition and ::setPosition', () =>
       it('sets the position of the window, and can retrieve the position just set', () => {
@@ -142,26 +142,39 @@ xdescribe("the `NylasEnv` global", function nylasEnvSpec() {
       expect(extra.column).toBe(3)
     });
 
-    it("Catches unhandled rejections", () => {
+    it("Catches unhandled rejections", async () => {
       spyOn(NylasEnv, "reportError");
       const err = new Error("TEST");
-      runs(() => {
+
-        const p = new Promise((resolve, reject) => {
+      const p = new Promise((resolve, reject) => {
-          setTimeout(() => {
+        reject(err);
-            reject(err);
-          }, 10)
-        })
-        p.then(() => {
-          throw new Error("Shouldn't resolve")
-        })
-        advanceClock(10);
       })
-      waitsFor(() => NylasEnv.reportError.callCount > 0);
+      p.then(() => {
-      runs(() => {
+        throw new Error("Shouldn't resolve")
-        expect(NylasEnv.reportError.callCount).toBe(1);
-        expect(NylasEnv.reportError.calls[0].args[0]).toBe(err);
-        expect(NylasEnv.reportError.calls[0].args[1].promise).toBeDefined();
       })
+
+      /**
+       * This test was started from within the `setTimeout` block of the
+       * Node event loop. The unhandled rejection will not get caught
+       * until the "pending callbacks" block (which happens next). Since
+       * that happens immediately next it's important that we don't use:
+       *
+       * await new Promise(setImmediate)
+       *
+       * Because of setImmediate's position in the Node event loop
+       * relative to this test and process.on('unhandledRejection'), using
+       * setImmediate would require us to await for it twice.
+       *
+       * We can use the original, no-stubbed-out `setTimeout` to put our
+       * test in the correct spot in the Node event loop relative to
+       * unhandledRejection.
+       */
+      await new Promise((resolve) => {
+        window.originalSetTimeout(resolve, 0)
+      })
+
+      expect(NylasEnv.reportError.callCount).toBe(1);
+      expect(NylasEnv.reportError.calls[0].args[0]).toBe(err);
     });
 
     describe("reportError", () => {

src/nylas-env.coffee

@@ -232,18 +232,23 @@ class NylasEnvConstructor
       originalError.stack = convertStackTrace(originalError.stack, sourceMapCache)
       @reportError(originalError, {url, line, column})
 
-    Promise.onPossiblyUnhandledRejection (error, promise) =>
-      error.stack = convertStackTrace(error.stack, sourceMapCache)
-
-      # API Errors are logged to Sentry only under certain circumstances,
-      # and are logged directly from the NylasAPI class.
-      if error instanceof APIError
-        return if error.statusCode isnt 400
-
-      @reportError(error)
-
     process.on('uncaughtException', (e) => @reportError(e))
-    process.on('unhandledRejection', (e) => @reportError(e))
+
+    # We use the native Node 'unhandledRejection' instead of Bluebird's
+    # `Promise.onPossiblyUnhandledRejection`. Testing indicates that
+    # the Node process method is a strict superset of Bluebird's handler.
+    # With the introduction of transpiled async/await, it is now possible
+    # to get a native, non-Bluebird Promise. In that case, Bluebird's
+    # `onPossiblyUnhandledRejection` gets bypassed and we miss some
+    # errors. The Node process handler catches all Bluebird promises plus
+    # those created with a native Promise.
+    process.on('unhandledRejection', (error) =>
+      if @inDevMode()
+        error.stack = convertStackTrace(error.stack, sourceMapCache)
+        @reportError(error)
+      else
+        @reportError(error)
+    )
 
     if @inSpecMode() or @inDevMode()
       Promise.longStackTraces()