Make sure TypeErrors thrown by frozen are caught

[astorije]

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]

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]

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]

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]

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]

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

[astorije]

@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]

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]

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]

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]

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

[astorije]

Knock knock, someone in here? :)

[keithamus]

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

[astorije]

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