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

Using td.replace() with external modules #51

Closed
jzaefferer opened this Issue Nov 18, 2015 · 16 comments

Comments

Projects
None yet
6 participants
@jzaefferer
Copy link

jzaefferer commented Nov 18, 2015

Based on the node example I've tried to use td.replace() with a external module. The module I'm testing does something like this:

import elasticsearch from 'elasticsearch'
var client = new elasticsearch.Client({ ... })
client.search({ ... });

My test, currently using sinon, does something like this:

import sinon from 'sinon'
import elasticsearch from 'elasticsearch'
searchSpy = {
  search: sinon.stub().returns(Promise.resolve({
    hits: {
      hits: [ ... ]
    }
  }))
}
sinon.stub(elasticsearch, 'Client').returns(searchSpy)

Stubbing only elasticsearch.Client is something I want to get rid of (to solve an unrelated problem). td.replace() seems like the right tool for that. So I tried something like this:

import elasticsearch from 'elasticsearch'
import td from 'testdouble'
searchSpy = {
  search: sinon.stub().returns(Promise.resolve({
    hits: {
      hits: [ ... ]
    }
  }))
}
var es = td.replace('elasticsearch')
td.when(es.Client()).return(searchSpy)

That fails with TypeError: es.Client is not a function. td.when(es.Client) doesn't work either.

Since there is no documentation for td.replace() and the example only imports internal module (with relative paths), I can't tell if I'm doing it wrong (likely) or if this is just not supported (yet, also likely).

If the snippets above aren't enough, I can also put together a sample that actually works.

@jzaefferer

This comment has been minimized.

Copy link

jzaefferer commented Nov 20, 2015

PS: I got a response on Twitter, saying that testdouble and quibble (the underlying lib) won't support this. More details coming eventually. I'll leave this open since the docs, once they exist, should mention the limitations.

@searls

This comment has been minimized.

Copy link
Member

searls commented Nov 20, 2015

Hey Jörn, you're absolutely right. Sorry for the delay / I plumb forgot because I've been traveling.

The tl;dr edition of why quibble doesn't support replacing modules is that when we practice TDD with test doubles, we "don't mock what we don't own". The reason for using mocks in our practice is to improve the richness of design feedback by making awkward interactions with dependencies painful; the appropriate response to that pain is to make that dependency's API better. With that being the purpose of using test doubles, replacing a third-party API will often just lead to useless pain, because the author isn't in an immediate position to improve the third party API.

Rather, our use of 3rd party dependencies (insofar as how our unit tests interact with them) typically break down into two categories:

  • Utility functions (e.g. lodash) — we call through to the real utility function from our unit test so long as they don't add additional side effects or break the purity of the function; I use these much more in unit tests of pure functions, which themselves typically don't require any dependencies (and therefore test doubles) at all
  • Integration functions (e.g. request) — we create adapters/wrappers of these dependencies that delegate to the third-party functions and then we replace those adapters/wrappers in our tests. That way, any custom branching, configuration, or API de-awkward-ification we apply to that dependency is in the wrapper and effectively centralized in a single place in the app. Not only is this a great way to prevent a third party module from seeping throughout the codebase, but it can provide a template for how to later replace that area of functionality with a different 3rd party dependency. It can even be a good first step towards introducing an adapter pattern to deal with an option of multiple 3rd party dependencies.

As a result, we aren't planning to go out of our way to support replacing modules with quibble / td.js

@searls

This comment has been minimized.

Copy link
Member

searls commented Nov 20, 2015

Because it's not super clear above, we absolutely will update the docs and we'll leave this open until we do

@searls searls closed this in 938fa36 Nov 28, 2015

@smeijer

This comment has been minimized.

Copy link

smeijer commented Apr 9, 2016

Is there any chance that this decision will be reconsidered? Testing meteor code requires stubbing out core packages. Currently I use a combination of proxyquire and sinon.js, but I have to admit that the api of testdouble looks very promising. If only td.replace('meteor/meteor', td.object['userId']) would have let me replaced the Meteor object.

It would be great if we could replace:

// ...
const { post, savePost } = proxyquire('../post.js', {
  'meteor/meteor': { Meteor, '@noCallThru': true },
  'meteor/kadira:flowrouter': { FlowRouter, '@noCallThru': true },
});

with

// ...
td.replace('meteor/meteor', Meteor);
td.replace('meteor/kadira:flowrouter', FlowRouter);
import { post, savePost } from '../post.js';
@searls

This comment has been minimized.

Copy link
Member

searls commented Apr 9, 2016

Take a look at the quibble module and take a stab at a pull request. If you can implement this with decent tests and without adding too much complexity, I'll consider a merge.

On Apr 8, 2016, at 20:18, smeijer notifications@github.com wrote:

Is there any chance that this decision will be reconsidered? Testing meteor code requires stubbing out core packages. Currently I use a combination of proxyquire and sinon.js, but I have to admit that the api of testdouble looks very promising. If only td.replace('meteor/meteor', td.object['userId']) would have let me replaced the Meteor object.

It would be great if we could replace:

// ...
const { post, savePost } = proxyquire('../post.js', {
'meteor/meteor': { Meteor, '@noCallThru': true },
'meteor/kadira:flowrouter': { FlowRouter, '@noCallThru': true },
});
with

// ...
td.replace('meteor/meteor', Meteor);
td.replace('meteor/kadira:flowrouter', FlowRouter);
import { post, savePost } from '../post.js';

You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub

@FagnerMartinsBrack

This comment has been minimized.

Copy link

FagnerMartinsBrack commented Apr 9, 2016

@smeijer I agree with @searls that one should not mock dependencies that you don't own. But then this fall appart when you assume that you shouldn't mock any dependency that you don't own, that is not the case.

There are use cases for mocking external dependencies, but that should only be done when you are unit testing the internal abstraction that serves as an interface to the external dependency. In other words, you should not mock dependencies you don't own, unless you are testing the module that was created for the sole reason of not mocking the external dependencies in the first place :)

The library should definitely not encourage bad code, but should be liberal in what it exposes if there are valid use cases that could be blocked due to an intended limitation. If it is possible to mock external dependencies using testdouble.js, the library should do so for the exceptions, but it should serve as a guideline to mocking and document the cases when it is a bad practice and when it is not.

I'm very sorry that I am not able to help with code right now, but I hope this was useful.

@smeijer

This comment has been minimized.

Copy link

smeijer commented Apr 9, 2016

@FagnerMartinsBrack & @searls , no worries. coffeescript is not really my thing, so I cannot help for the pull request on this feature. But in the mean time I found out that for meteor there is a working solution by using testdouble.

You can read more on it on the meteor forum, where I explain it in a post of mine.

To give a short summary; we cannot use an import statement like the sample above, because babel moves the import statements to the top on the file while transpiling. But when using require, it does work.

not working:

td.replace('meteor/meteor', Meteor);
td.replace('meteor/kadira:flowrouter', FlowRouter);
import { post, savePost } from '../post.js';

working:

td.replace('meteor/meteor', { Meteor });
td.replace('meteor/kadira:flowrouter', { FlowRouter });
const { post, savePost } = require('../post.js');
@mightyiam

This comment has been minimized.

Copy link

mightyiam commented Oct 18, 2016

@searls

This comment has been minimized.

Copy link
Member

searls commented Nov 7, 2016

should only be done when you are unit testing the internal abstraction that serves as an interface to the external dependency

FWIW, I disagree with this statement. While it feels good to have an isolated unit test of every single file in a system, wrapper/adapter units don't benefit from being isolated from the 3rd party dependency. I recommend either testing them so as to call through to the external dep or rely on incidental coverage via an integrated test.

Mocking out a third party library doesn't give any actionable design feedback, since the system you're testing isn't in a position to change the API. More, the job of the wrapper is to provide a consistent API to that third party thing, so its test should verify that responsibility by making sure the adapter/wrapper works as intended.

@FagnerMartinsBrack

This comment has been minimized.

Copy link

FagnerMartinsBrack commented Nov 8, 2016

@searls There are 2 main purposes of testing with third party code: document the expectation of an internal API regarding its dependencies and ensure it works with them.

Unit tests are meant to document expectations and integration tests are meant to help to verify the correctness of the system when integrated with the third party (despite which API is being used, integration tests don't care about that).

An integration test satisfies the requirements of a unit test, but have drawbacks because it:

  • Does not provide the root cause of the problem
  • Is more expensive to run

A unit test is incompetent for ensuring correctness because it doesn't run against the real third party , but it:

  • Allows documenting what is the expectation of the third party API
  • Runs faster, giving immediate feedback
  • Presents the root cause of the issue when it fails

We can mix both and have a test that calls an internal unit and test it against the third party directly without having to mock it, so feedback will be received exactly in the internal unit that is being tested, even if it's integrating with the third party directly, then it saves us from this drawback:

  • Does not provide the root cause of the problem

If we don't rely on a third party that does IO or other sort of expensive operation, then we can reasonably consider that the other drawback is also fixed because the run time can be fast:

  • Is more expensive to run

In this case, when we can test the unit and the third party is not expensive and we want to ensure correctness, then it makes sense not using mocks.

However, if we are using a third party that is expensive and we want to document a specific expectation between a third party and our domain without caring about their internal implementation (upgrading the tests once the API releases a new version with a proper changelog) then it makes sense using mocks for a third party.

I am not saying that we should always or never use mocks. Neither I am saying that we should never or always use the real third party. What I am saying is that there's no silver bullet and there are use cases for both.

@indolering

This comment has been minimized.

Copy link

indolering commented Nov 18, 2016

@FagnerMartinsBrack it sounds like TestDouble isn't opposed to a PR adding support for this feature - they just don't want to put much work into it. Perhaps you could contribute some code?

@FagnerMartinsBrack

This comment has been minimized.

Copy link

FagnerMartinsBrack commented Nov 18, 2016

@indolering

it sounds like TestDouble isn't opposed to a PR adding support for this feature - they just don't want to put much work into it

Is there a comment on this thread saying that?

Perhaps you could contribute some code?

I would definitely do that if I didn't have a lot of side projects in my hands. Open Source is supposed to be about the small things that together sums up as the whole. I believe comments are also valuable and unfortunately, that's the only thing I can do for the time being.

I hope it was useful.

@indolering

This comment has been minimized.

Copy link

indolering commented Nov 19, 2016

it sounds like TestDouble isn't opposed to a PR adding support for this feature - they just don't want to put much work into it

Is there a comment on this thread saying that?

Quoting @searls:

we aren't planning to go out of our way to support replacing modules with quibble / td.js
...
If you can implement this with decent tests and without adding too much complexity, I'll consider a merge.

We're all super busy, I came here to clarify this point as well. At least one project considers the lack of such functionality as a blocker. Hopefully someone can get paid to add this feature 😄.

@FagnerMartinsBrack

This comment has been minimized.

Copy link

FagnerMartinsBrack commented Nov 19, 2016

@indolering I guess I missed that, thanks for pointing it out. Nevertheless, for the reasons above, I can't tackle this now =/

Bountysource might be a good alternative, it would probably help if everyone on this thread give a few bucks for it given that it's probably being found by organic traffic or issues searching.

@searls

This comment has been minimized.

Copy link
Member

searls commented Nov 19, 2016

Yeah, we are open to it, but we still have the concerns we did at the outset--that giving folks a module replacement method would encourage replacing third party dependencies directly without wrapping them. At the same time that's not always necessary (e.g. a separate module that you do control)

Anyway maybe someone has the time to look into adding this to quibble over the holiday.

@searls

This comment has been minimized.

Copy link
Member

searls commented Mar 25, 2017

Hey @jzaefferer and friends, I've changed my mind about this. Enough teams are using private modules at this point as a code organization strategy, that no one is benefited by the library preventing using td.replace in this way, especially because it requires hardly any code changes on our part to support this. (Really it just requires resolving from the perspective of the current working directory).

See #220

@searls searls closed this in #220 Mar 25, 2017

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