From 7e70503fec6c57e3e128e4b6f8f938e74e5da381 Mon Sep 17 00:00:00 2001
From: Joel Bradshaw <joel@vivor.com>
Date: Wed, 3 Nov 2021 10:41:57 -0700
Subject: [PATCH 1/5] 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"

Also, fix typo that inverted the meaning of a comment :)
---
 lib/sinon/proxy-call.js |   2 +-
 test/assert-test.js     | 182 +++++++++-------------------------------
 2 files changed, 40 insertions(+), 144 deletions(-)

diff --git a/lib/sinon/proxy-call.js b/lib/sinon/proxy-call.js
index ef81e8806..e1a5d991d 100644
--- a/lib/sinon/proxy-call.js
+++ b/lib/sinon/proxy-call.js
@@ -220,7 +220,7 @@ var callProto = {
             }
         }
         if (this.stack) {
-            // Omit the error message and the two top stack frames in sinon itself:
+            // Emit the error message and the two top stack frames in sinon itself:
             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 95e49d8ad..e571d2247 100644
--- a/test/assert-test.js
+++ b/test/assert-test.js
@@ -1438,7 +1438,8 @@ describe("assert", function () {
                         [].slice.call(arguments, 1)
                     );
                 } catch (e) {
-                    return e.message;
+                    // Strip off the stack frames we append
+                    return e.message.replace(/( at.*\(.*\)$)+/gm, "");
                 }
             };
         });
@@ -1454,10 +1455,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 +1467,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 +1481,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,10 +1565,7 @@ 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()"
             );
         });
@@ -1586,20 +1575,14 @@ describe("assert", function () {
             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 +1592,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 +1604,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 +1692,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 +1704,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 +1790,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 +1801,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 +1817,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 +1829,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 +1840,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 +1851,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 +1862,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 +1874,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 +1888,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 +1900,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 +1911,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 +1922,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 +1934,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 +1949,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 +1965,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 +1982,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 +1999,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 +2022,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 +2039,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 +2054,7 @@ describe("assert", function () {
                     1,
                     3,
                     "bob"
-                ).replace(/ at.*/g, ""),
+                ),
                 "expected doSomething to be called once and with exact arguments "
             );
 
@@ -2156,7 +2066,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 +2074,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"
@@ -2198,7 +2105,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" +
@@ -2215,12 +2122,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)"
             );
         });
@@ -2234,7 +2136,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)"
             );
         });
@@ -2244,10 +2146,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)"
             );
         });
@@ -2257,10 +2156,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)"
             );
         });

From 42ac44fd7ce285fdafab60b0b3f9a861c1fea1c5 Mon Sep 17 00:00:00 2001
From: Joel Bradshaw <joel@vivor.com>
Date: Wed, 3 Nov 2021 10:47:54 -0700
Subject: [PATCH 2/5] 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!
---
 lib/sinon/assert.js | 14 ++++++++++----
 test/assert-test.js |  9 +++++++++
 2 files changed, 19 insertions(+), 4 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/test/assert-test.js b/test/assert-test.js
index e571d2247..f59dff901 100644
--- a/test/assert-test.js
+++ b/test/assert-test.js
@@ -1570,6 +1570,15 @@ describe("assert", function () {
             );
         });
 
+        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();

From 98a180454f32deb9ca4d7c8f4028486c8a142d25 Mon Sep 17 00:00:00 2001
From: Joel Bradshaw <joel@vivor.com>
Date: Wed, 3 Nov 2021 11:47:53 -0700
Subject: [PATCH 3/5] A little more explanatory comment

---
 test/assert-test.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/assert-test.js b/test/assert-test.js
index f59dff901..a0db5c896 100644
--- a/test/assert-test.js
+++ b/test/assert-test.js
@@ -1438,7 +1438,8 @@ describe("assert", function () {
                         [].slice.call(arguments, 1)
                     );
                 } catch (e) {
-                    // Strip off the stack frames we append
+                    // We sometimes append stack frames to the message and they
+                    // make assertions messy, so strip those off here
                     return e.message.replace(/( at.*\(.*\)$)+/gm, "");
                 }
             };

From 078f3a8a11c43ae9d17180a7d2db939c64cfc18a Mon Sep 17 00:00:00 2001
From: Joel Bradshaw <joel@vivor.com>
Date: Wed, 19 Jan 2022 13:29:41 -0800
Subject: [PATCH 4/5] Re-fix the comment about appending stack frames

The previous "fix" was wrong, I misunderstood this code and the comment.
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.
---
 lib/sinon/proxy-call.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/sinon/proxy-call.js b/lib/sinon/proxy-call.js
index e1a5d991d..e76e93dcd 100644
--- a/lib/sinon/proxy-call.js
+++ b/lib/sinon/proxy-call.js
@@ -220,7 +220,8 @@ var callProto = {
             }
         }
         if (this.stack) {
-            // Emit 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 "

From cda8c8580d2d2671db8ea7f31d518a617c2c7e85 Mon Sep 17 00:00:00 2001
From: Joel Bradshaw <joel@vivor.com>
Date: Wed, 19 Jan 2022 16:48:52 -0800
Subject: [PATCH 5/5] 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
---
 test/proxy-call-test.js | 41 ++++++++++++++++++++++++++++-------------
 1 file changed, 28 insertions(+), 13 deletions(-)

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 () {