From 56b06129e223eae690265c37b1113067e2b31bdc Mon Sep 17 00:00:00 2001 From: Joel Bradshaw Date: Thu, 20 Jan 2022 06:41:18 -0800 Subject: [PATCH] Check call count type (#2410) * Strip stack frames in `this.message` This saves us from having to do it every time, and makes things much nicer. Also use a little bit more specific regex, to avoid issues with messages that happen to contain the word "at" * Check type of callCount argument and error accordingly This is to fixes #2408, which could result in error messages like "expected spy to be called 10 times but was called 10 times". Now we will instead say "expected '10' to be a number, but was of type string", which is much clearer! * A little more explanatory comment * Edit the comment about appending stack frames What's actually happening here is that we want to add a frame of context to `callStr`, but the first two stack frames will be within Sinon code and thus probably not helpful to the end-user. So, we skip the first two stack frames, and append the third stack frame, which should contain a meaningful location to the end-user. * Add test for adding stack traces to error message This ensures that if at some point we end up with another Sinon layer in the stack at some point, we'll catch it and hopefully adjust accordingly For reference, as of this commit, the Sinon portion of the stack is: lib/sinon/proxy-invoke.js:65:15 lib/sinon/proxy.js:265:26 Also convert a neighboring test to async while we're at it --- lib/sinon/assert.js | 14 ++- lib/sinon/proxy-call.js | 3 +- test/assert-test.js | 192 ++++++++++------------------------------ test/proxy-call-test.js | 41 ++++++--- 4 files changed, 89 insertions(+), 161 deletions(-) diff --git a/lib/sinon/assert.js b/lib/sinon/assert.js index ffd34e550..5af71bbb6 100644 --- a/lib/sinon/assert.js +++ b/lib/sinon/assert.js @@ -159,10 +159,16 @@ function createAssertObject() { callCount: function assertCallCount(method, count) { verifyIsStub(method); - if (method.callCount !== count) { - var msg = `expected %n to be called ${timesInWords( - count - )} but was called %c%C`; + var msg; + if (typeof count !== "number") { + msg = + `expected ${format(count)} to be a number ` + + `but was of type ${typeof count}`; + failAssertion(this, msg); + } else if (method.callCount !== count) { + msg = + `expected %n to be called ${timesInWords(count)} ` + + `but was called %c%C`; failAssertion(this, method.printf(msg)); } else { assert.pass("callCount"); diff --git a/lib/sinon/proxy-call.js b/lib/sinon/proxy-call.js index ef81e8806..e76e93dcd 100644 --- a/lib/sinon/proxy-call.js +++ b/lib/sinon/proxy-call.js @@ -220,7 +220,8 @@ var callProto = { } } if (this.stack) { - // Omit the error message and the two top stack frames in sinon itself: + // If we have a stack, add the first frame that's in end-user code + // Skip the first two frames because they will refer to Sinon code callStr += (this.stack.split("\n")[3] || "unknown").replace( /^\s*(?:at\s+|@)?/, " at " diff --git a/test/assert-test.js b/test/assert-test.js index c686026c0..deb09381e 100644 --- a/test/assert-test.js +++ b/test/assert-test.js @@ -1438,7 +1438,9 @@ describe("assert", function () { [].slice.call(arguments, 1) ); } catch (e) { - return e.message; + // We sometimes append stack frames to the message and they + // make assertions messy, so strip those off here + return e.message.replace(/( at.*\(.*\)$)+/gm, ""); } }; }); @@ -1454,10 +1456,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message("notCalled", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("notCalled", this.obj.doSomething), "expected doSomething to not have been called but was called once\n doSomething()" ); }); @@ -1469,10 +1468,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message("notCalled", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("notCalled", this.obj.doSomething), "expected doSomething to not have been called " + "but was called 4 times\n doSomething()\n " + "doSomething()\n doSomething()\n doSomething()" @@ -1486,10 +1482,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message("notCalled", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("notCalled", this.obj.doSomething), "expected doSomething to not have been called " + "but was called 4 times\n doSomething()\n " + "doSomething(3)\n doSomething(42, 1)\n doSomething()" @@ -1573,33 +1566,33 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message("callCount", this.obj.doSomething, 3).replace( - / at.*/g, - "" - ), + this.message("callCount", this.obj.doSomething, 3), "expected doSomething to be called thrice but was called once\n doSomething()" ); }); + it("assert.callCount exception message with non-numeric argument", function () { + this.obj.doSomething(); + + assert.equals( + this.message("callCount", this.obj.doSomething, "3"), + "expected '3' to be a number but was of type string" + ); + }); + it("assert.calledOnce exception message", function () { this.obj.doSomething(); this.obj.doSomething(); assert.equals( - this.message("calledOnce", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("calledOnce", this.obj.doSomething), "expected doSomething to be called once but was called twice\n doSomething()\n doSomething()" ); this.obj.doSomething(); assert.equals( - this.message("calledOnce", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("calledOnce", this.obj.doSomething), "expected doSomething to be called once but was called " + "thrice\n doSomething()\n doSomething()\n doSomething()" ); @@ -1609,10 +1602,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message("calledTwice", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("calledTwice", this.obj.doSomething), "expected doSomething to be called twice but was called once\n doSomething()" ); }); @@ -1624,10 +1614,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message("calledThrice", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("calledThrice", this.obj.doSomething), "expected doSomething to be called thrice but was called 4 times\n" + " doSomething()\n doSomething()\n doSomething()\n doSomething()" ); @@ -1715,13 +1702,7 @@ describe("assert", function () { this.obj.doSomething(4, 3, "hey"); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - 1, - 3, - "hey" - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, 1, 3, "hey"), `expected doSomething to be called with arguments \n${color.red( "4" )} ${color.green("1")} \n3\n${inspect('"hey"')}` @@ -1733,13 +1714,7 @@ describe("assert", function () { this.obj.doSomething(1, 3, "not"); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - 1, - 3, - "hey" - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, 1, 3, "hey"), `${ "expected doSomething to be called with arguments \n" + "Call 1:\n" @@ -1825,10 +1800,7 @@ describe("assert", function () { this.obj.doSomething(4); assert.equals( - this.message("calledWith", this.obj.doSomething, 1, 3).replace( - / at.*/g, - "" - ), + this.message("calledWith", this.obj.doSomething, 1, 3), `expected doSomething to be called with arguments \n${color.red( "4" )} ${color.green("1")} \n${color.green("3")}` @@ -1839,10 +1811,7 @@ describe("assert", function () { this.obj.doSomething(4, 3); assert.equals( - this.message("calledWith", this.obj.doSomething, 1).replace( - / at.*/g, - "" - ), + this.message("calledWith", this.obj.doSomething, 1), `expected doSomething to be called with arguments \n${color.red( "4" )} ${color.green("1")} \n${color.red("3")}` @@ -1858,7 +1827,7 @@ describe("assert", function () { this.obj.doSomething, match.any, false - ).replace(/ at.*/g, ""), + ), `${ "expected doSomething to be called with arguments \n" + "true any\n" @@ -1870,11 +1839,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - match.defined - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, match.defined), `expected doSomething to be called with arguments \n ${color.red( "defined" )}` @@ -1885,11 +1850,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - match.truthy - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, match.truthy), `expected doSomething to be called with arguments \n ${color.red( "truthy" )}` @@ -1900,11 +1861,7 @@ describe("assert", function () { this.obj.doSomething(true); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - match.falsy - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, match.falsy), `expected doSomething to be called with arguments \n${color.green( "true" )} ${color.red("falsy")}` @@ -1915,11 +1872,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - match.same(1) - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, match.same(1)), `expected doSomething to be called with arguments \n ${color.red( "same(1)" )}` @@ -1931,11 +1884,7 @@ describe("assert", function () { var matcher = match.typeOf("string"); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - matcher - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, matcher), `expected doSomething to be called with arguments \n ${color.red( 'typeOf("string")' )}` @@ -1949,11 +1898,7 @@ describe("assert", function () { }); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - matcher - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, matcher), `expected doSomething to be called with arguments \n ${color.red( "instanceOf(CustomType)" )}` @@ -1965,11 +1910,7 @@ describe("assert", function () { var matcher = match({ some: "value", and: 123 }); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - matcher - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, matcher), `expected doSomething to be called with arguments \n ${color.red( "match(some: value, and: 123)" )}` @@ -1980,11 +1921,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - match(true) - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, match(true)), `expected doSomething to be called with arguments \n ${color.red( "match(true)" )}` @@ -1995,11 +1932,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - match(123) - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, match(123)), `expected doSomething to be called with arguments \n ${color.red( "match(123)" )}` @@ -2011,11 +1944,7 @@ describe("assert", function () { var matcher = match("Sinon"); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - matcher - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, matcher), `expected doSomething to be called with arguments \n ${color.red( 'match("Sinon")' )}` @@ -2030,7 +1959,7 @@ describe("assert", function () { "calledWith", this.obj.doSomething, match(/[a-z]+/) - ).replace(/ at.*/g, ""), + ), `expected doSomething to be called with arguments \n ${color.red( "match(/[a-z]+/)" )}` @@ -2046,11 +1975,7 @@ describe("assert", function () { }); assert.equals( - this.message( - "calledWith", - this.obj.doSomething, - matcher - ).replace(/ at.*/g, ""), + this.message("calledWith", this.obj.doSomething, matcher), `expected doSomething to be called with arguments \n ${color.red( "match(custom)" )}` @@ -2067,7 +1992,7 @@ describe("assert", function () { 4, 3, "hey" - ).replace(/ at.*/g, ""), + ), `expected doSomething to be called with match \n${color.red( "1" )} ${color.green("4")} \n3\n${inspect('"hey"')}` @@ -2084,7 +2009,7 @@ describe("assert", function () { this.obj.doSomething, 1, "hey" - ).replace(/ at.*/g, ""), + ), `${ "expected doSomething to always be called with arguments \n" + "Call 1:\n" + @@ -2107,7 +2032,7 @@ describe("assert", function () { this.obj.doSomething, 1, "hey" - ).replace(/ at.*/g, ""), + ), `${ "expected doSomething to always be called with match \n" + "Call 1:\n" + @@ -2124,12 +2049,7 @@ describe("assert", function () { this.obj.doSomething(1, 3, "hey"); assert.equals( - this.message( - "calledWithExactly", - this.obj.doSomething, - 1, - 3 - ).replace(/ at.*/g, ""), + this.message("calledWithExactly", this.obj.doSomething, 1, 3), `expected doSomething to be called with exact arguments \n1\n3\n${color.red( inspect('"hey"') )}` @@ -2144,7 +2064,7 @@ describe("assert", function () { 1, 3, "bob" - ).replace(/ at.*/g, ""), + ), "expected doSomething to be called once and with exact arguments " ); @@ -2156,7 +2076,7 @@ describe("assert", function () { 1, 3, "bob" - ).replace(/ at.*/g, ""), + ), `expected doSomething to be called once and with exact arguments \n${color.red( "4" )} ${color.green("1")} \n3\n${inspect('"bob"')}` @@ -2164,10 +2084,7 @@ describe("assert", function () { this.obj.doSomething(); assert.equals( - this.message( - "calledOnceWithExactly", - this.obj.doSomething - ).replace(/ at.*/g, ""), + this.message("calledOnceWithExactly", this.obj.doSomething), `${ "expected doSomething to be called once and with exact arguments \n" + "Call 1:\n" @@ -2210,7 +2127,7 @@ describe("assert", function () { this.obj.doSomething, 1, 3 - ).replace(/ at.*/g, ""), + ), `${ "expected doSomething to always be called with exact arguments \n" + "Call 1:\n" + @@ -2227,12 +2144,7 @@ describe("assert", function () { this.obj.doSomething(1, 2, 3); assert.equals( - this.message( - "neverCalledWith", - this.obj.doSomething, - 1, - 2 - ).replace(/ at.*/g, ""), + this.message("neverCalledWith", this.obj.doSomething, 1, 2), "expected doSomething to never be called with arguments 1, 2\n doSomething(1, 2, 3)" ); }); @@ -2246,7 +2158,7 @@ describe("assert", function () { this.obj.doSomething, 1, 2 - ).replace(/ at.*/g, ""), + ), "expected doSomething to never be called with match 1, 2\n doSomething(1, 2, 3)" ); }); @@ -2256,10 +2168,7 @@ describe("assert", function () { this.obj.doSomething(1, 3); assert.equals( - this.message("threw", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("threw", this.obj.doSomething), "doSomething did not throw exception\n doSomething(1, 3, 'hey')\n doSomething(1, 3)" ); }); @@ -2269,10 +2178,7 @@ describe("assert", function () { this.obj.doSomething(1, 3); assert.equals( - this.message("alwaysThrew", this.obj.doSomething).replace( - / at.*/g, - "" - ), + this.message("alwaysThrew", this.obj.doSomething), "doSomething did not always throw exception\n doSomething(1, 3, 'hey')\n doSomething(1, 3)" ); }); diff --git a/test/proxy-call-test.js b/test/proxy-call-test.js index 5b8dd5438..a9871a11a 100644 --- a/test/proxy-call-test.js +++ b/test/proxy-call-test.js @@ -1176,7 +1176,7 @@ describe("sinonSpy.call", function () { // https://github.com/sinonjs/sinon/issues/1066 /* eslint-disable consistent-return */ - it("does not throw when the call stack is empty", function (done) { + it("does not throw when the call stack is empty", async function () { if (typeof Promise === "undefined") { this.skip(); } @@ -1184,21 +1184,36 @@ describe("sinonSpy.call", function () { var stub1 = sinonStub().resolves(1); var stub2 = sinonStub().returns(1); - function run() { - return stub1().then(stub2); - } + await stub2(await stub1()); - run() - .then(function () { - assert.equals( - stub2.getCall(0).toString().replace(/ at.*/g, ""), - "stub(1) => 1" - ); - done(); - }) - .catch(done); + assert.equals( + stub2.getCall(0).toString().replace(/ at.*/g, ""), + "stub(1) => 1" + ); }); /* eslint-enable consistent-return */ + + it("includes first stack entry from end-user code", function () { + /* We find the first stack frame that points to end-user code and + * add it to the error message. We do this by chopping off a + * constant number of stack frames, so if this test fails, you + * probably need to chop off a different number of frames + */ + if (typeof __filename === "undefined") { + this.skip(); + } + + var object = { doIt: sinonSpy() }; + object.doIt(); + + const [name, stackFrame] = object.doIt + .getCall(0) + .toString() + .split(" at "); + + assert.equals(name, "doIt()"); + assert.contains(stackFrame, __filename); + }); }); describe("constructor", function () {