Figure out babel+instanceof+constructor problems with class extension

[searls]

Moving #157 here now that we know what's going on.

Three things at once going on here: when you replace a class, td.js will extend that class and give you a fake one.

1. we extend the class so the fake will pass instanceof checks in the subject. This was raised by several issues in td 1.x and was the main motivation for the change
2. because of ES rules, if that class has a custom constructor, subclass constructors must call super, which sucks because a fake thing shouldn't be invoking a real constructor
3. because of how babel transpiles, faking a real class will extend it from a not-real (transpiled babel) class, which calls the super constructor but without the new keyword, which raises an error under v8 versions that support the class keyword

Dupes: #240

[razor-x]

Not sure if this is the same bug, but in ~2.0.1 to ~2.1.2 there was a breaking regression with how td.object works with classes. Here is minimal example to reproduce the issue: https://github.com/rxbugs/testdouble-object-class

[searls]

I don't know what the cause here is, but could you try pulling down testdouble.js, link this example to the lib and then git bisect it to find the offending hash? (Btw, @razor-x, this is almost certainly unrelated)

[searls]

Ok, somewhat frustratingly this fix doesn't work in babel

[razor-x]

❤️ git bisect

works: c54b741
breaks: 1ca7fc1

1ca7fc1cef8c8f19080bf258e7d7760387b39391 is the first bad commit
commit 1ca7fc1cef8c8f19080bf258e7d7760387b39391
Author: Justin Searls <searls@gmail.com>
Date:   Wed Mar 29 14:37:06 2017 -0400

    Improve error message with invalid td.object call
    
    Fixes #224

:040000 040000 417c2737711b1505c07558cea72c3d7900620549 3fea1b41ba13bf8004c6da5d34cc7c3d12db7c2c M	src
:040000 040000 942823ab0ecc93b3ee181f894f580523affd1c96 0eb449cccc2a8ee7ebd296bce5f696f9f530f503 M	test

[searls]

I'm sorry for the delay in getting back to you. This is pretty confusing so strap in.

My preliminary conclusion after looking at this more closely is that you were, in a sense, using our ES Proxy support in an ambiguous way that we didn't intend to actually support.

If you look at this line of your demo project, you stub foo.bar() successfully, but because you'd been handed an ES Proxy your test that the object was actually being doubled was a false positive.

To illustrate, in your "working" example, I could get away with this:

const assert = require('assert')

const td = require('testdouble')

class Foo {
  bar () {
    return false
  }
}

const doBar = foo => foo.lololol()

const foo = td.object(new Foo())
td.when(foo.lololol()).thenReturn(true)
assert.ok(doBar(foo))

This is clearly not what you'd expect, because you're handing td.object an object, and you'd expect its methods are being doubled. Except you're not. You're getting a proxy object that doesn't have any awareness of Foo.

Instead, what I'd typically recommend is that you td.constructor(Foo) and then create an instance of that fake. There are enough special cases around instances of ES classes, that we've previously decided not to support passing them to td.object()

As a result of the above I'm closing this issue, but am open to hearing your reaction

[razor-x]

Thanks for the response. As I didn't get any feedback before I submitted the PR assuming a regression.

The lololol example makes sense, does as you said, and yes I would want that to fail. I can look at switching to using td.constructor but I need to make sure it works as expected.

Something like this makes sense to me and would require minimal refactoring to change:

const assert = require('assert')

const td = require('testdouble')

class Foo {
  bar () {
    return false
  }
}

const doBar = foo => foo.bar()

const foo = new td.constructor(Foo)
td.when(foo.bar()).thenReturn(true)
assert.ok(doBar(foo))

but it fails with

/home/razorx/devel/bugs/testdouble-object-class/working/index.js:14
td.when(foo.bar()).thenReturn(true)
            ^

TypeError: foo.bar is not a function

Also, trying to mimic the example in the docs

const assert = require('assert')

const td = require('testdouble')

class Foo {
  bar () {
    return false
  }
}

const doBar = foo => foo.bar()

const FakeFoo = td.constructor(Foo)
td.when(FakeFoo.prototype.bar()).thenReturn(true)
assert.ok(doBar(new FakeFoo()))

fails with

/home/razorx/devel/bugs/testdouble-object-class/working/node_modules/testdouble/lib/constructor.js:41
      var _this = _possibleConstructorReturn(this, (TestDoubleConstructor.__proto__ || Object.getPrototypeOf(TestDoubleConstructor)).apply(this, arguments));
                                                                                                                                     ^

TypeError: Class constructor Foo cannot be invoked without 'new'
    at new TestDoubleConstructor (/home/razorx/devel/bugs/testdouble-object-class/working/node_modules/testdouble/lib/constructor.js:41:134)
    at Object.<anonymous> (/home/razorx/devel/bugs/testdouble-object-class/working/index.js:15:17)
    at Module._compile (module.js:571:32)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:488:32)
    at tryModuleLoad (module.js:447:12)
    at Function.Module._load (module.js:439:3)
    at Module.runMain (module.js:605:10)
    at run (bootstrap_node.js:427:7)
    at startup (bootstrap_node.js:151:9)

[searls]

Yes, the last failure is the issue that I thought this issue was all about to begin with and needs to be addressed with some kind of solution like #242

[searls]

For the meantime, you can just change your test to td.object('Foo') and get a proxy object named Foo that will behave exactly as it had previously

[searls]

Resolved by #254 and landed in testdouble@3.0