[Discussion] Expect only overrides error stack for built-in matchers (#5162)

* Expect only overrides error stack for built-in matchers

* Fixed naggy lint issue

* Replaced for...of with Object.keys().forEach()

* Removed JestAssertionError export (as it is no longer needed)

* Added custom matcher test

* Adjusted custom_matcher test to use prettified, non-absolute stack

* test: refactor integration test

* update with correct snapshot

* test: skip on windows

* Clarified comment

* Further clarified comment

* test: fix test for node 6

* test: move custom matcher stack to separate file to be able to skip it on windows

* Revert "test: fix test for node 6"

This reverts commit f902b4b.

* test: really fix node 6

* docs: add package prefix to changelog

also format all markdown files

* Reverted unrelated Markdown changes that had made their way into the branch

* Replaced '__jestInternal' string attribute with a Symbol

* Updated snapshot

Author: bvaughn

CHANGELOG.md

@@ -9,6 +9,8 @@
   Windows platform. ([#5161](https://github.com/facebook/jest/pull/5161))
 * `[jest-regex-util]` Fix breaking change in `--testPathPattern`
   ([#5230](https://github.com/facebook/jest/pull/5230))
+* `[expect]` Do not override `Error` stack (with `Error.captureStackTrace`) for
+  custom matchers. ([#5162](https://github.com/facebook/jest/pull/5162))
 
 ### Features
 

integration_tests/__tests__/__snapshots__/custom_matcher_stack_trace.test.js.snap

@@ -0,0 +1,29 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`works with custom matchers 1`] = `
+"FAIL __tests__/custom_matcher.test.js
+  Custom matcher
+    ✓ passes
+    ✓ fails
+    ✕ preserves error stack
+
+  ● Custom matcher › preserves error stack
+
+    qux
+
+      43 |     const bar = () => baz();
+      44 |     const baz = () => {
+    > 45 |       throw Error('qux');
+      46 |     };
+      47 | 
+      48 |     // This expecation fails due to an error we throw (intentionally)
+      
+      at __tests__/custom_matcher.test.js:45:13
+      at __tests__/custom_matcher.test.js:43:23
+      at __tests__/custom_matcher.test.js:42:23
+      at __tests__/custom_matcher.test.js:52:7
+      at __tests__/custom_matcher.test.js:11:18
+      at __tests__/custom_matcher.test.js:53:8
+
+"
+`;

integration_tests/__tests__/__snapshots__/failures.test.js.snap

@@ -118,7 +118,7 @@ exports[`works with async failures 1`] = `
     +   \\"foo\\": \\"bar\\",
       }
       
-      at ../../packages/expect/build/index.js:155:54
+      at ../../packages/expect/build/index.js:156:54
 
 "
 `;

integration_tests/__tests__/custom_matcher_stack_trace.test.js

@@ -0,0 +1,28 @@
+/**
+ * Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+'use strict';
+
+const runJest = require('../runJest');
+const {extractSummary} = require('../utils');
+const skipOnWindows = require('../../scripts/skip_on_windows');
+
+skipOnWindows.suite();
+
+test('works with custom matchers', () => {
+  const {stderr} = runJest('custom_matcher_stack_trace');
+
+  let {rest} = extractSummary(stderr);
+
+  rest = rest
+    .split('\n')
+    .filter(line => line.indexOf('at Error (native)') < 0)
+    .join('\n');
+
+  expect(rest).toMatchSnapshot();
+});

integration_tests/custom_matcher_stack_trace/__tests__/custom_matcher.test.js

@@ -0,0 +1,55 @@
+/**
+ * Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ */
+'use strict';
+
+function toCustomMatch(callback, expectation) {
+  const actual = callback();
+
+  if (actual !== expectation) {
+    return {
+      message: () => `Expected "${expectation}" but got "${actual}"`,
+      pass: false,
+    };
+  } else {
+    return {pass: true};
+  }
+}
+
+expect.extend({
+  toCustomMatch,
+});
+
+describe('Custom matcher', () => {
+  it('passes', () => {
+    // This expectation should pass
+    expect(() => 'foo').toCustomMatch('foo');
+  });
+
+  it('fails', () => {
+    expect(() => {
+      // This expectation should fail,
+      // Which is why it's wrapped in a .toThrow() block.
+      expect(() => 'foo').toCustomMatch('bar');
+    }).toThrow();
+  });
+
+  it('preserves error stack', () => {
+    const foo = () => bar();
+    const bar = () => baz();
+    const baz = () => {
+      throw Error('qux');
+    };
+
+    // This expecation fails due to an error we throw (intentionally)
+    // The stack trace should point to the line that throws the error though,
+    // Not to the line that calls the matcher.
+    expect(() => {
+      foo();
+    }).toCustomMatch('test');
+  });
+});

integration_tests/custom_matcher_stack_trace/package.json

@@ -0,0 +1,5 @@
+{
+  "jest": {
+    "testEnvironment": "node"
+  }
+}

packages/expect/src/__tests__/stacktrace.test.js

@@ -9,6 +9,18 @@
 const jestExpect = require('../');
 
 jestExpect.extend({
+  toCustomMatch(callback, expectation) {
+    const actual = callback();
+
+    if (actual !== expectation) {
+      return {
+        message: () => `Expected "${expectation}" but got "${actual}"`,
+        pass: false,
+      };
+    }
+
+    return {pass: true};
+  },
   toMatchPredicate(received, argument) {
     argument(received);
     return {
@@ -22,7 +34,7 @@ it('stack trace points to correct location when using matchers', () => {
   try {
     jestExpect(true).toBe(false);
   } catch (error) {
-    expect(error.stack).toContain('stacktrace.test.js:23');
+    expect(error.stack).toContain('stacktrace.test.js:35');
   }
 });
 
@@ -32,6 +44,22 @@ it('stack trace points to correct location when using nested matchers', () => {
       jestExpect(value).toBe(false);
     });
   } catch (error) {
-    expect(error.stack).toContain('stacktrace.test.js:32');
+    expect(error.stack).toContain('stacktrace.test.js:44');
+  }
+});
+
+it('stack trace points to correct location when throwing from a custom matcher', () => {
+  try {
+    jestExpect(() => {
+      const foo = () => bar();
+      const bar = () => baz();
+      const baz = () => {
+        throw new Error('Expected');
+      };
+
+      foo();
+    }).toCustomMatch('bar');
+  } catch (error) {
+    expect(error.stack).toContain('stacktrace.test.js:57');
   }
 });

packages/expect/src/index.js

@@ -34,6 +34,7 @@ import {
   stringMatching,
 } from './asymmetric_matchers';
 import {
+  INTERNAL_MATCHER_FLAG,
   getState,
   setState,
   getMatchers,
@@ -214,6 +215,7 @@ const makeThrowingMatcher = (
       result = matcher.apply(matcherContext, [actual].concat(args));
     } catch (error) {
       if (
+        matcher[INTERNAL_MATCHER_FLAG] === true &&
         !(error instanceof JestAssertionError) &&
         error.name !== 'PrettyFormatPluginError' &&
         // Guard for some environments (browsers) that do not support this feature.
@@ -252,7 +254,8 @@ const makeThrowingMatcher = (
   };
 };
 
-expect.extend = (matchers: MatchersObject): void => setMatchers(matchers);
+expect.extend = (matchers: MatchersObject): void =>
+  setMatchers(matchers, false);
 
 expect.anything = anything;
 expect.any = any;
@@ -280,9 +283,9 @@ const _validateResult = result => {
 };
 
 // add default jest matchers
-expect.extend(matchers);
-expect.extend(spyMatchers);
-expect.extend(toThrowMatchers);
+setMatchers(matchers, true);
+setMatchers(spyMatchers, true);
+setMatchers(toThrowMatchers, true);
 
 expect.addSnapshotSerializer = () => void 0;
 expect.assertions = (expected: number) => {

packages/expect/src/jest_matchers_object.js

@@ -13,6 +13,10 @@ import type {MatchersObject} from 'types/Matchers';
 // the state, that can hold matcher specific values that change over time.
 const JEST_MATCHERS_OBJECT = Symbol.for('$$jest-matchers-object');
 
+// Notes a built-in/internal Jest matcher.
+// Jest may override the stack trace of Errors thrown by internal matchers.
+export const INTERNAL_MATCHER_FLAG = Symbol.for('$$jest-internal-matcher');
+
 if (!global[JEST_MATCHERS_OBJECT]) {
   Object.defineProperty(global, JEST_MATCHERS_OBJECT, {
     value: {
@@ -35,6 +39,13 @@ export const setState = (state: Object) => {
 
 export const getMatchers = () => global[JEST_MATCHERS_OBJECT].matchers;
 
-export const setMatchers = (matchers: MatchersObject) => {
+export const setMatchers = (matchers: MatchersObject, isInternal: boolean) => {
+  Object.keys(matchers).forEach(key => {
+    const matcher = matchers[key];
+    Object.defineProperty(matcher, INTERNAL_MATCHER_FLAG, {
+      value: isInternal,
+    });
+  });
+
   Object.assign(global[JEST_MATCHERS_OBJECT].matchers, matchers);
 };