-
-
Notifications
You must be signed in to change notification settings - Fork 699
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
Make sure TypeErrors thrown by frozen are caught #496
Conversation
b4bd9b3
to
3c380df
Compare
Sorry, I don't think I made myself clear enough in the issue. We shouldn't throw TypeErrors in the chai codebase, only AssertionErrors. In the example comment I made, the catch did a Assertion.addProperty('frozen', function() {
var obj = flag(this, 'object');
// try/catch because calls to Object.isFrozen in ES5 will throw TypeErrors on non-objects
try {
Object.isFrozen(obj)
this.assert(
Object.isFrozen(obj)
, 'expected #{this} to be frozen'
, 'expected #{this} to not be frozen'
);
} catch(e) {
this.assert(
false
, 'expected #{this} to be frozen, but got #{act}'
, 'expected #{this} to not be frozen but got #{act}'
, obj
, e
);
}); Also - once again me not being clear - this will need to be done for all of this type of method - |
Hi @keithamus, thanks for the quick feedback!
I think you did actually, and both complaints you have were done on purpose from my end, so clearly I didn't present my case well enough. Allow me to try to do that here.
I figured that's what you wanted according to your code sample, but I wonder why we should not throw a Another approach we could choose is to adopt the ES6-for-all strategy: if a
Oh noooo, my fault for not specifying that in the PR description! Of course I'll submit a PR for all of these properties, I just wanted you to agree on my changes or to settle any discussion on one property before copying and pasting everything! Sorry I didn't mention that earlier... |
I disagree here - if we're throwing errors that aren't AssertionErrors, it means we have failed as an assertion lib. It is no concern to the developer how we test that their object is frozen, just that we only throw an AssertionError with a good explanation of why this test fails. If we're just throwing a TypeError with a different message, we may as well just not catch the TypeError and let it throw up to the user the original error. I feel like we're try/catching so we can present the user with something more fitting (an AssertionError). Having said all of that, your suggestion of emulating ES6 behaviour is one I'm keen on. I can see it raising some issues in the issue tracker, but I think in the long run it makes the most sense. We just need to be careful of false positives. If you want to refactor the PRs to do this, please do so, just make sure you have lots of tests 😄 |
My feeling was that everything that fails because the assertion is wrong should be an Alright alright... but note that we have to ensure it more globally then :-) (see #501)
Yes, this looks both elegant and odd :-) Deal then, I'll update the PR soon. |
👍 good work so far @astorije. Look forward to seeing the updated PR 😄 |
cffb96b
to
fcb3354
Compare
@keithamus I made the changes and added tests, for |
Note that my null.should.be.frozen;
undefined.should.be.frozen; both give me:
I have never used |
fcb3354
to
b1df871
Compare
b1df871
to
eae67c0
Compare
Everything looks really good @astorije. 👍 * 💯 Don't worry about Do you want to continue adding support for extensible/sealed in this PR? If not I can merge this one and you can set up new ones for those. |
Well that's sad, at least for the
I am right now, will wrap this up quite soon. |
There @keithamus, I am done. Please don't hesitate if I forgot or overlooked something! |
Knock knock, someone in here? :) |
Sorry for the delay @astorije. Fantastic work, excellent research, and thank you for your patience! |
Make sure TypeErrors thrown by frozen are caught
Thanks for your kind words @keithamus, that's greatly appreciated! No worries for the delay ;) |
See #491.
Example of a failure is:
I would have preferred to hide the stack trace but I'm not sure one can easily do it with a
TypeError
, and in this case I think aTypeError
makes much more sense than anAssertionError
.I didn't investigate much, but where is it that you manage to not show the stack trace for
AssertionError
s?