Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Check call count type #2410

Merged
merged 5 commits into from Jan 20, 2022
Merged

Check call count type #2410

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7e70503
Strip stack frames in `this.message`
cincodenada Nov 3, 2021
42ac44f
Check type of callCount argument and error accordingly
cincodenada Nov 3, 2021
98a1804
A little more explanatory comment
cincodenada Nov 3, 2021
078f3a8
Re-fix the comment about appending stack frames
cincodenada Jan 19, 2022
cda8c85
Add test for adding stack traces to error message
cincodenada Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 10 additions & 4 deletions lib/sinon/assert.js
View file
Open in desktop
Expand Up @@ -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");
Expand Down
2 changes: 1 addition & 1 deletion lib/sinon/proxy-call.js
View file
Open in desktop
Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change/fix is wrong. Looking at the code, it does omit the first three elements of this.stack, which would be [message, stackframe1, stackframe2]`, so I think it was correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, yes upon investigation you're totally right here, thanks! I misunderstood what was going on here. I misread in a couple ways:

  • I read this as a comment about what the code is doing, not how it's doing it. (It's adding things, not omitting them - but it is omitting things from the things that it's adding. Phew!)
  • I read "in sinon itself" as "add the stack frame to sinon's output" when I now realize it probably means "omit the top two stack frames, which will be in sinon itself".

After re-orienting myself, I do still think it's a little confusing. One more small bit, we're actually just pulling out only the third stack frame, right? We're not just stripping off the first two and leaving the rest, I think?

Anyway, this is a lot of focus on a small change, and the comment as it was is certainly more accurate than my replacement now that I understand it correctly. So we could revert and leave well enough alone and that would be fine! But what do you think of something like:

          if (this.stack) {
            // If present, add the third stack frame to the output for context
            // Skip the first two stack frames because they will be within Sinon code
            callStr += (this.stack.split("\n")[3] || "unknown").replace(
                /^\s*(?:at\s+|@)?/,
                " at "
            );
          }

(Code is the same, I just left it there for context)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely an improvement!

callStr += (this.stack.split("\n")[3] || "unknown").replace(
/^\s*(?:at\s+|@)?/,
" at "
Expand Down