Skip to content
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

[BREAKING] Return constructor for td.replace() #166

Closed
4 tasks
searls opened this issue Dec 14, 2016 · 0 comments · Fixed by #193
Closed
4 tasks

[BREAKING] Return constructor for td.replace() #166

searls opened this issue Dec 14, 2016 · 0 comments · Fixed by #193
Projects
Milestone

Comments

@searls
Copy link
Member

searls commented Dec 14, 2016

There is an impedance mismatch in the current 1.x series API for td.replace that is causing enough confusion and edge case pain that I think we should unwind it.

First, an example to anyone not familiar:

Example

Suppose we have module './gamepad':

import Button from './button'

module.exports = function () {
  new Button().press('A')
}

If we're testing Gamepad and replacing Button

function test() {
  var button = td.replace('./button')
  var subject = require('./gamepad')

  subject()

  td.verify(button.press('A'))
}

In the above case, button is essentially set to {press: td.func('Button#press')}.

However, in the './gamepad' module, a constructor for Button will be provided when it's imported/required, so that the line new Button().press('A') will behave the same way in production as it does when under test.

This was done because a unit test typically just wants to get at the "instance" methods on the type that's being replaced and to have to reach into the prototype by doing: td.verify(button.prototype.press('A')) was initially seen as unnecessarily awkward.

The case for changing this

A few reasons we want to change this:

  • This is surprising behavior to everyone but @searls
  • Returning the same thing to the test and the subject will be more obvious/honest/symmetrical for users
  • This will allow us to refactor one of the ugliest parts of td.js's codebase
  • "static" methods, or properties on the constructor function (as opposed to the prototype) are neither faked nor returned to the test
  • This may enable "instanceof" if we set up prototypal inheritance between our fake constructors and the constructor being doubled.
  • This may enable us to allow users to ensure replaced dependencies are instantiated as expected, which couldn't be done before because the constructor function itself wasn't faked and returned

Requirements

  • Figure out how to stub/verify the constructor itself to be sure the subject instantiates the dependency with the expected params. I have no idea how we'll do this, but this is probably our only chance of figuring that out. I'd like to see a few examples of this that accomplish what a reasonable user would want before we decide on a final API
  • Ensure the final API works will pass instanceof checks in subject code (e.g. if I td.replace PantsError then error instanceof PantsError should return true for the fake (and not for other arbitrary faked constructors)
  • Ensure the final API works with ES6 "static" methods
  • Cut a migration release that warns with instructions for how to fix this and, potentially, an intermediary API folks can opt-into and then simply do a grep-replace when they do upgrade to 2.x

This will obviate the need for implementing the following as separate features: #54, #159, #164, #121, #108

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

1 participant