-
-
Notifications
You must be signed in to change notification settings - Fork 694
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.
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
feat: Add the assert.fail([message]) interface #1117
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1117 +/- ##
==========================================
+ Coverage 93.64% 93.68% +0.03%
==========================================
Files 32 32
Lines 1637 1646 +9
Branches 393 396 +3
==========================================
+ Hits 1533 1542 +9
Misses 104 104
Continue to review full report at Codecov.
|
84a378e
to
8f7addc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me! Ideally, this PR will also need to change should.fail
and expect.fail
though.
OK @keithamus, I will change |
Done. I noticed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s-leroux Thanks for contributing!
I have a few inline comments regarding this PR. Also, my initial feeling is that this constitutes a breaking change, since it's changing how parameter handling is performed on an existing assertion. If other maintainers agree that this is the case, then prior to being merged, the commits in this PR should be squashed into a single one with a commit message body that begins with "BREAKING CHANGE:" (commit message title can remain as-is).
There may also need to be some follow-up actions regarding TypeScript per #1119, but I don't think that needs to be resolved in this PR.
@@ -41,6 +41,7 @@ module.exports = function (chai, util) { | |||
}; | |||
|
|||
/** | |||
* ### .fail([message]) | |||
* ### .fail(actual, expected, [message], [operator]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chaijs/chai Anyone know how the website generation script handles listing multiple function signatures in this way? Or the proper way to do this with jsdoc in general? I struggled with this problem quite a bit in chaijs/check-error#18, and ended up including a single function signature at the top (with optional parameters) but documenting the alternate signatures in the jsdoc body. Not sure what the best approach is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a possible solution for handling multiple function signature used in the assert.throws
documentation. But I'm not sure this is the proper way of doing though: as a user, this is only by looking at the example section I was able to understand the various signatures for that method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I passed the source code through dox
(which I assume to be the jsdoc parser used for the website). Here is the relevant part of the output:
"description": {
"full": "<h3>.fail([message])</h3>\n<h3>.fail(actual, expected, [message], [operator])</h3>\n<p>Throw a failure.</p>",
"summary": "<h3>.fail([message])</h3>\n<h3>.fail(actual, expected, [message], [operator])</h3>",
"body": "<p>Throw a failure.</p>"
},
As you can see, it produces two <h3>
elements--once for each signature. Which seems quite reasonnable. Maybe we could ping someone at the chai-doc team to check if this is an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, chai-docs is using dox
. That behavior looks promising. @astorije might know for sure.
@@ -10,6 +10,7 @@ module.exports = function (chai, util) { | |||
}; | |||
|
|||
/** | |||
* ### .fail([message]) | |||
* ### .fail(actual, expected, [message], [operator]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here regarding function signatures.
@@ -42,6 +42,7 @@ module.exports = function (chai, util) { | |||
var should = {}; | |||
|
|||
/** | |||
* ### .fail([message]) | |||
* ### .fail(actual, expected, [message], [operator]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here regarding function signatures.
test/assert.js
Outdated
@@ -20,6 +20,18 @@ describe('assert', function () { | |||
}).to.throw(chai.AssertionError, /this has failed/); | |||
}); | |||
|
|||
it('fail', function () { | |||
chai.expect(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rater than using chai.expect.to.throw
, I think we can use the same err()
helper function for these assert
interface tests that are used in the expect
and should
interface tests. Doing so has the advantage of the test making sure implementation frames aren't in the Assertion Error's stack trace.
test/assert.js
Outdated
@@ -20,6 +20,18 @@ describe('assert', function () { | |||
}).to.throw(chai.AssertionError, /this has failed/); | |||
}); | |||
|
|||
it('fail', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like the test descriptions to be more descriptive than "fail", even if it's as simple as "fail (1 arg)" and "fail (no args)". Another option would be to create a new describe
block for fail
, and then put descriptive it
blocks inside of it.
test/assert.js
Outdated
chai.expect(function () { | ||
assert.fail(); | ||
}).to.throw(chai.AssertionError, /assert\.fail/); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as above regarding description and err
helper function.
test/expect.js
Outdated
}, 'this has failed'); | ||
}); | ||
|
||
it('fail', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the assert
interface test regarding it
block description.
test/expect.js
Outdated
}, 'this has failed'); | ||
}); | ||
|
||
it('fail', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the assert
interface test regarding it
block description.
test/should.js
Outdated
@@ -239,6 +239,18 @@ describe('should', function() { | |||
}, 'this has failed'); | |||
}); | |||
|
|||
it('fail', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the assert
interface test regarding it
block description.
test/should.js
Outdated
}, 'this has failed'); | ||
}); | ||
|
||
it('fail', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the assert
interface test regarding it
block description.
My feeling is this is a feature. My reasoning is that the behaviour of |
@keithamus Good point. I agree. Not a breaking change. |
First, thank you @keithamus and @meeber for having reviewed that PR.
Of course.
I will fix that. I just copied-pasted existing tests. And I assumed there was some obscure reason for not using it in the
I have no idea how to do it properly. Any suggestion welcome!
For now, I have absolutly no idea what your talking about ;)
I will do that once I will have your green light for the other changes. |
bde6607
to
8a987b4
Compare
I fixed the tests, add a little bit of documentation in the method header and squashed the commit. I left the "double function signature" in the method header since this is definitely the most understandable way to describe the different function signatures and that seems to be supported by
But let me know if you want I change that to something else, in case this would cause an issue with the chai-doc tool. |
Thanks @s-leroux. I like the changes. I'd say let's move forward with the double function signatures unless someone chimes in otherwise. One thing: there's a couple of duplicate tests that can be deleted. |
Fix chaijs#1116. The `assert.fail` interface should accept being called with only 1 arguments to fail with a custom message.
8a987b4
to
0d1b586
Compare
Thank you for your review @meeber. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great work @s-leroux! Really clean PR with excellent tests, incuding bumping existing coverage. Also thank you so much for responding to our reviews so quickly and properly! You should consider yourself proud!
Thank you @keithamus. It was a pleasure! |
In relation to #1116 the
assert.fail
interface should accept only 1 arguments to fail with a custom message.