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

Merged
merged 4 commits into from Aug 10, 2015

Conversation

Projects
None yet
2 participants
@astorije
Member

astorije commented Jul 21, 2015

See #491.

Example of a failure is:

  1) expect frozen:
     TypeError: The element must be a non-null object or an array, number given.
      at Assertion.<anonymous> (lib/chai/core/assertions.js:1733:15)
      at Assertion.Object.defineProperty.get (lib/chai/utils/addProperty.js:35:29)
      at Context.<anonymous> (test/expect.js:1129:20)

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 a TypeError makes much more sense than an AssertionError.

I didn't investigate much, but where is it that you manage to not show the stack trace for AssertionErrors?

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 22, 2015

Member

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 this.assert which was destined to fail, intentionally so, because then we can throw a AssertionError rather than a TypeError. I'll paste the example here:

  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 - sealed, frozen and extensible all behave the same way, and all need this try catch clause to handle the TypeErrors.

Member

keithamus commented Jul 22, 2015

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 this.assert which was destined to fail, intentionally so, because then we can throw a AssertionError rather than a TypeError. I'll paste the example here:

  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 - sealed, frozen and extensible all behave the same way, and all need this try catch clause to handle the TypeErrors.

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Jul 22, 2015

Member

Hi @keithamus, thanks for the quick feedback!

Sorry, I don't think I made myself clear enough in the issue.

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.

We shouldn't throw TypeErrors in the chai codebase, only AssertionErrors. In the example comment I made, the catch did a this.assert which was destined to fail, intentionally so, because then we can throw a AssertionError rather than a TypeError.

I figured that's what you wanted according to your code sample, but I wonder why we should not throw a TypeError. If I am testing expect(true).to.be.false;, yes, clearly I deserve an AssertionError because, well, the assertion of saying that true is false is wrong. On the other end, if I am testing expect(true).to.be.frozen;, I violate the expected type of the isFrozen method. The answer should not be "yes, this assertion is true" or "no, this assertion is false", but really "sorry, this assertion is not even valid". In that way, I think it's a valid TypeError as Errors should always be thrown with semantics in mind.
Does all of that make sense to you? I can use other arguments, but if this one doesn't work, there is nothing I can do :-)
That being said, I'll do as you command (otherwise you won't merge anyway ^^), I just want us to settle on the right decision, whichever it is.

Another approach we could choose is to adopt the ES6-for-all strategy: if a TypeError is thrown, then .frozen should evaluate to true (which is what .frozen will do under ES6 anyway). It saves us from potential inconsistencies but, I don't know, it doesn't seem right to me.
A scenario I could think of is testing that the returned value of a function should be a frozen object, but it returns a boolean instead. If one doesn't check the actual value but just if it's frozen or not, this will be a false positive although the code itself might actually fail. On the other hand, if you don't test the actual value that gets returned, I'm not sure you deserve your program to run :-)
Again, you tell me.

Also - once again me not being clear - this will need to be done for all of this type of method - sealed, frozen and extensible all behave the same way, and all need this try catch clause to handle the TypeErrors.

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...

Member

astorije commented Jul 22, 2015

Hi @keithamus, thanks for the quick feedback!

Sorry, I don't think I made myself clear enough in the issue.

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.

We shouldn't throw TypeErrors in the chai codebase, only AssertionErrors. In the example comment I made, the catch did a this.assert which was destined to fail, intentionally so, because then we can throw a AssertionError rather than a TypeError.

I figured that's what you wanted according to your code sample, but I wonder why we should not throw a TypeError. If I am testing expect(true).to.be.false;, yes, clearly I deserve an AssertionError because, well, the assertion of saying that true is false is wrong. On the other end, if I am testing expect(true).to.be.frozen;, I violate the expected type of the isFrozen method. The answer should not be "yes, this assertion is true" or "no, this assertion is false", but really "sorry, this assertion is not even valid". In that way, I think it's a valid TypeError as Errors should always be thrown with semantics in mind.
Does all of that make sense to you? I can use other arguments, but if this one doesn't work, there is nothing I can do :-)
That being said, I'll do as you command (otherwise you won't merge anyway ^^), I just want us to settle on the right decision, whichever it is.

Another approach we could choose is to adopt the ES6-for-all strategy: if a TypeError is thrown, then .frozen should evaluate to true (which is what .frozen will do under ES6 anyway). It saves us from potential inconsistencies but, I don't know, it doesn't seem right to me.
A scenario I could think of is testing that the returned value of a function should be a frozen object, but it returns a boolean instead. If one doesn't check the actual value but just if it's frozen or not, this will be a false positive although the code itself might actually fail. On the other hand, if you don't test the actual value that gets returned, I'm not sure you deserve your program to run :-)
Again, you tell me.

Also - once again me not being clear - this will need to be done for all of this type of method - sealed, frozen and extensible all behave the same way, and all need this try catch clause to handle the TypeErrors.

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...

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 24, 2015

Member

I figured that's what you wanted according to your code sample, but I wonder why we should not throw a TypeError. If I am testing expect(true).to.be.false;, yes, clearly I deserve an AssertionError because, well, the assertion of saying that true is false is wrong.

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 😄

Member

keithamus commented Jul 24, 2015

I figured that's what you wanted according to your code sample, but I wonder why we should not throw a TypeError. If I am testing expect(true).to.be.false;, yes, clearly I deserve an AssertionError because, well, the assertion of saying that true is false is wrong.

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 😄

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Jul 25, 2015

Member

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).

My feeling was that everything that fails because the assertion is wrong should be an AssertionError, and everything that fails because of a violation of the expect() method should be treated differently, being a mistake from the developer. But I understand this is arguable, such as if the parameter given to expect() is the return of a function and this function mistakenly returns the wrong type, we wouldn't want chai to explode.

Alright alright... but note that we have to ensure it more globally then :-) (see #501)

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 😄

Yes, this looks both elegant and odd :-)
I keep thinking of problems it can create, but the only scenarios I can think of involve bad test suites. I guess scenarios will come up as GH issues if people ever get stuck. Feel free to @mention me when this happens!
Of course I'll be as complete as possible in the tests ;-)

Deal then, I'll update the PR soon.

Member

astorije commented Jul 25, 2015

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).

My feeling was that everything that fails because the assertion is wrong should be an AssertionError, and everything that fails because of a violation of the expect() method should be treated differently, being a mistake from the developer. But I understand this is arguable, such as if the parameter given to expect() is the return of a function and this function mistakenly returns the wrong type, we wouldn't want chai to explode.

Alright alright... but note that we have to ensure it more globally then :-) (see #501)

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 😄

Yes, this looks both elegant and odd :-)
I keep thinking of problems it can create, but the only scenarios I can think of involve bad test suites. I guess scenarios will come up as GH issues if people ever get stuck. Feel free to @mention me when this happens!
Of course I'll be as complete as possible in the tests ;-)

Deal then, I'll update the PR soon.

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 25, 2015

Member

👍 good work so far @astorije. Look forward to seeing the updated PR 😄

Member

keithamus commented Jul 25, 2015

👍 good work so far @astorije. Look forward to seeing the updated PR 😄

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Jul 26, 2015

Member

@keithamus I made the changes and added tests, for frozen only. Let me know if this looks good so that I can reproduce to sealed and extensible or make changes.

Member

astorije commented Jul 26, 2015

@keithamus I made the changes and added tests, for frozen only. Let me know if this looks good so that I can reproduce to sealed and extensible or make changes.

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Jul 26, 2015

Member

Note that my should tests are lacking a few things, as the following 2 lines:

    null.should.be.frozen;
    undefined.should.be.frozen;

both give me:

  1) should frozen:
     TypeError: Cannot read property 'should' of null

I have never used should before, is there a caveat I should be aware of, in order to test these primitives too? (Wrapping them in var myVar = null didn't help)

Member

astorije commented Jul 26, 2015

Note that my should tests are lacking a few things, as the following 2 lines:

    null.should.be.frozen;
    undefined.should.be.frozen;

both give me:

  1) should frozen:
     TypeError: Cannot read property 'should' of null

I have never used should before, is there a caveat I should be aware of, in order to test these primitives too? (Wrapping them in var myVar = null didn't help)

@keithamus keithamus referenced this pull request Jul 26, 2015

Closed

Chai and ES6 polyfills #502

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Jul 26, 2015

Member

Everything looks really good @astorije. 👍 * 💯

Don't worry about null.should and undefined.should - pretty much everyone who uses should for any amount of time understands that it can cause those types of errors.

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.

Member

keithamus commented Jul 26, 2015

Everything looks really good @astorije. 👍 * 💯

Don't worry about null.should and undefined.should - pretty much everyone who uses should for any amount of time understands that it can cause those types of errors.

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.

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Jul 26, 2015

Member

Don't worry about null.should and undefined.should - pretty much everyone who uses should for any amount of time understands that it can cause those types of errors.

Well that's sad, at least for the null part. But as long as it's tested in either of the should or expect APIs, we should be safe.

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.

I am right now, will wrap this up quite soon.

Member

astorije commented Jul 26, 2015

Don't worry about null.should and undefined.should - pretty much everyone who uses should for any amount of time understands that it can cause those types of errors.

Well that's sad, at least for the null part. But as long as it's tested in either of the should or expect APIs, we should be safe.

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.

I am right now, will wrap this up quite soon.

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Jul 26, 2015

Member

There @keithamus, I am done. Please don't hesitate if I forgot or overlooked something!

Member

astorije commented Jul 26, 2015

There @keithamus, I am done. Please don't hesitate if I forgot or overlooked something!

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Aug 8, 2015

Member

Knock knock, someone in here? :)

Member

astorije commented Aug 8, 2015

Knock knock, someone in here? :)

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Aug 10, 2015

Member

Sorry for the delay @astorije. Fantastic work, excellent research, and thank you for your patience!

Member

keithamus commented Aug 10, 2015

Sorry for the delay @astorije. Fantastic work, excellent research, and thank you for your patience!

keithamus added a commit that referenced this pull request Aug 10, 2015

Merge pull request #496 from astorije/astorije/frozen-errors
Make sure TypeErrors thrown by frozen are caught

@keithamus keithamus merged commit a42ac43 into chaijs:master Aug 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@astorije astorije deleted the astorije:astorije/frozen-errors branch Aug 10, 2015

@astorije

This comment has been minimized.

Show comment
Hide comment
@astorije

astorije Aug 10, 2015

Member

Thanks for your kind words @keithamus, that's greatly appreciated! No worries for the delay ;)

Member

astorije commented Aug 10, 2015

Thanks for your kind words @keithamus, that's greatly appreciated! No worries for the delay ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment