Could you try to explain the problem and the solution presented here. The current version of equal, takes two values of type T1 and T2, then a common super type and use that for equality. If you call equal from within the method, this process if repeated. That seems like pretty reasonable implementation, so I would like to know how this is different.

@sunesimonsen The issue is that the Error type defers to it's baseType .equal() method. Once inside the baseType (in this case object), keys are retrieved using the object type .getKeys() method which does not account for potential enumerability differences in Error objects and thus fails. I noted an example that should succeed but currently fails in the issue I opened.

What I believe needs to happen is that the base object equality still needs to use the specific types of (a, b) to fetch their keys and that's what this patch does - it passes down the specific types of the two arguments such that key fetching happens using them. There is a good chance I'm not explaining it well - in that case I believe after our conversation yesterday @papandreou has an understanding of what's up and perhaps I could ask him to chime in :)

Yes, was just about to say that I see the problem caused by the call the base type and the taped on getKeys thing that was introduced at a later point :-)

I'll try to think about it, this PR touches a pretty fundamental part of Unexpected, so we should watch out for introducing even more complexity.

@sunesimonsen yep :-) and absolutely - please consider this a suggestion.

There was an alternative I considered, which would be to carve out all the equality code into a method on utils (so that would include the identity check, key fetching, length check etc) - that method would also take the type as an argument such that in this case the baseType.equal() call in the Error type would be replaced by something like utils.objectEqulity(errorType, ...) and thus you'd correctly fetch keys. What I couldn't convince myself of though is whether that'd work generally. I will note one of the reasons I thought about it was it'd avoid those extra args to Unexpected.prototype.equal(). The reason I made the extra argument an object in this PR though was so it's be a little less "one off" and reserve some wiggle room for the future.

This problem also affects the object type on its own when a property of either the lhs or rhs has been declared with Object.defineProperty(obj, {enumerable: false, ...}). That seems like something that shouldn’t cause equal To return false. We should make sure to solve that as well.

Looks like the same error happened with node 9 here: https://travis-ci.org/unexpectedjs/unexpected-http/jobs/357654632#L1736

@papandreou It's definitely a general thing - from what I've observed, version of node >=9.7.0 seem to attach certain error messages "after the fact" which makes them enumerable. I have a half written bug report that I intend to send over to them, but in the meantime my inclination would be that it's important to protect against this in core.

The attempt represented by this PR is one option, but I understand the concern around it - another option I floated earlier in the thread was to avoid the call into the super-type for the Error types by carving out object equality into a util method, something like objectsEqualByType method, and calling that explicitly. This would at least allow fixing the Error case.

The case I'm outlining should be fixed by removing the length check here, right?

Let's add a test like this and see if we can beat it into shape:

describe('with objects that differ only in the enumerability of keys', function () {
  it('should succeed when the lhs object has a non-enumerable key', function () {
    var a = { a: 123 };
    var b = { a: 123, b: 456 };
    Object.defineProperty(a, 'b', {
      value: 456,
      enumerable: false
    });
    expect(a, 'to equal', b);
  });

  it('should succeed when the rhs object has a non-enumerable key', function () {
    var a = { a: 123, b: 456 };
    var b = { a: 123 };
    Object.defineProperty(b, 'b', {
      value: 456,
      enumerable: false
    });
    expect(a, 'to equal', b);
  });
});

@papandreou with the object type hooks stuff done I'll look at picking this up again. Even if this is fixed by node (which it should have been but somehow it missed the last v9), I think it's worth hardening this. I've had an idea rattling about how to do this more cleanly, will move this back up "the list".

@alexjeffburke, great to hear that!

The attempted fix here is superseded by the comprehensive enumerable property support in #482. Closing this PR.