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

td.verify(object.function(new Class("param"))) not asserting "param" #362

Closed
jnssnmrcs opened this issue Apr 27, 2018 · 6 comments
Closed

Comments

@jnssnmrcs
Copy link

jnssnmrcs commented Apr 27, 2018

Description

Verifying a function has been called with a new instance of a class with specific values passed to constructor.

Issue

I don't know how to do this and did not manage to find any example explaining this.

Environment

node 8.9.0
npm 6.0.0
testdouble 3.7.0

Example Code

N/A

Runkit Notebook

N/A

Code-fenced Examples

client.js

function Security(username, password)
{
    this.username = username;
    this.password = password;
}

module.exports = {
    setSecurity: function(security)
    {
        // Do something
    },
    Security: Security
};

service.js (This is the module I want to test)

const client = require("./client");

module.exports = {
    createClient: function(username, password)
    {
        client.setSecurity(new client.Security(username, password));
    }
};

tests.js

const td = require("testdouble");

let client;
let service;

describe("service", function()
{
    beforeEach(function()
    {
        client = td.replace("./client");
        service = require("./service");
    });

    afterEach(function()
    {
        td.reset();
    });

    it("should create a new client", function()
    {
        service.createClient("username", "password");

        // Closest thing I've come up with so far
        td.verify(new client.Security("username", "password"));
        td.verify(client.setSecurity(td.matchers.isA(client.Security)));

        // What I would like to do which does not work.
        // It does not matter what parameters you pass to service.createClient,
        // it will always pass.
        td.verify(client.setSecurity(new client.Security("username", "password")));

        // Maybe the following better explains what I want to do, nesting td.verify().
        td.verify(client.setSecurity(td.verify(new client.Security("username", "password"))));
    });
});
@searls
Copy link
Member

searls commented Apr 28, 2018

Yeah, if this is all your subject does, then what you have here is the most you can do to specify the interaction you want:

td.verify(new client.Security("username", "password"), {times: 1});
td.verify(client.setSecurity(td.matchers.isA(client.Security)));

Since there is room for a disconnect between the two steps being verified, I added a verification option that client.Security was instantiated only once (that way the test will more-or-less prove that it was the instance with the correct user / pass that was passed into setSecurity).

There are two reasons why this is a pain point in the library and in faking JavaScript stuff generally:

  1. JavaScript objects instantiated with new are, best as I know, out of the hands of the function itself. We can extend and change a prototype for the runtime to clone, but we can't explicitly say "when called with new, return X" (that is a long way to say the return value of a new fun() call does not matter). As a result, the standard practice of td.when(login(user,pass)).thenReturn('woo') in order to chain a single outcome is not possible. Whoops! Turns out I actually built support for this and then forgot about it. See my next comment below.

  2. This function does not do anything but instantiate a value. In my own practice, I treat values differently than application/business logic units. Values should be easy to construct and wrap primitive data, with their only methods elucidating that data. As a result, there isn't anything to be gained by mocking them out. In fact, all you're likely to do is lose whatever type introspection and convenience you'd get from their structure, as it is in this case in which you can't just assert the username and password fields.

So when greeted with this pain point, what I would do is use this as feedback to pass real value types and fake out the things with logic.

In case this isn't clear, how I'd implement the same behavior given this observation follows, including new names to make it clear what does what (since it splits this client.js into two):

// client-credentials.js
module.exports = function ClientCredentials(username, password)
{
    this.username = username;
    this.password = password;
}
// sets-client-credentials.js
module.exports =  function(security) {}
// service.js
const ClientCredentials = require("./client-credentials");
const setsClientCredentials = require("./sets-client-credentials");

module.exports = {
    createClient: function(username, password)
    {
        setsClientCredentials(new ClientCredentials(username, password));
    }
};
// service-test.js
const td = require("testdouble");
const ClientCredentials = require("./client-credentials")

let setsClientCredentials, subject
describe("service", function()
{
    beforeEach(function()
    {
        setsClientCredentials = td.replace("./sets-client-credentials");
        subject = require("./service");
    });

    it("should set the credentials", function()
    {
        subject.createClient("username", "password");

        td.verify(setsClientCredentials(td.matchers.argThat(function(credential){
          return credential.username === 'username' && 
                     credential.password == 'password' &&
                     credential instanceof ClientCredentials
        })));
    });
});

You could, of course, verify this in a few different ways. You could use an argument captor instead, or you could have a more convenient way of comparing two value types. However, I wouldn't stop there!

I still wouldn't find myself writing tests like this much, because cases where I need to use advanced call verification features like argThat and captor are usually a smell that I'm over-relying on void functions that have a side effect when they could just as well return a value.

What I mean by this is that if I found myself writing this test, I think my next questions would be "why doesn't setsClientCredentials return a value? If it did, this would be easier to test because it'd be a simple stub & assertion, easier to debug because its result could be logged, and easier to read because I wouldn't need to go open its file listing to understand what it does".

This is the sort of thing I mean when I say that the purpose of TDD with isolated tests is to get "design feedback" about the usability of our private APIs. It's not often obvious that a very simple API (like the one you presented above), has some issues with clarity and maybe usability, but trying to write an isolated unit test to specify those (again, very simple) interactions was so painful that it certainly encourages one to start asking questions.

@searls searls closed this as completed Apr 28, 2018
@searls
Copy link
Member

searls commented Apr 28, 2018

By the way, if you haven't seen it yet, a lot of my discussion above might be informed by watching my recent talk on mocking: http://blog.testdouble.com/posts/2018-03-06-please-dont-mock-me

@jnssnmrcs
Copy link
Author

Thanks for an elaborate answer!

@searls
Copy link
Member

searls commented Apr 28, 2018

@jasonkarns just reminded me that ES constructors can indeed specify what they return so long as it's an object

And it turns out that testdouble.js actually supports this, I just always miss it because I usually try stubbing a string, which fails since it doesn't have the right prototype.

This means in your initial example, you could have done something like this instead of two verifications (warning: I flushed my sample project so I'm just guessing here that this should work based on a repl experiment):

const credentials = {username: 'a', password: 'b'}
td.when(new client.Security("username", "password")).thenReturn(credentials)

service.createClient("username", "password")

td.verify(client.setSecurity(credentials));

@jasonkarns
Copy link
Member

To be clear, a constructor (either class constructor or old-school constructor function) can return something other than the instance as long as the thing being returned is an object.

So returning a primitive from a constructor will fail, but returning any old object (including plain object literals) is fine. (They don't need to be subclasses of the constructor's parent.)

> class Bar {}
undefined
> class Foo extends Bar { constructor() { return {a:1} } }
undefined
> new Foo
{ a: 1 }

(Tested above using native node. All bets are off if this is getting compiled by babel or something.)

@jnssnmrcs
Copy link
Author

@jasonkarns just reminded me that ES constructors can indeed specify what they return so long as it's an object

And it turns out that testdouble.js actually supports this, I just always miss it because I usually try stubbing a string, which fails since it doesn't have the right prototype.

This means in your initial example, you could have done something like this instead of two verifications (warning: I flushed my sample project so I'm just guessing here that this should work based on a repl experiment):

const credentials = {username: 'a', password: 'b'}
td.when(new client.Security("username", "password")).thenReturn(credentials)

service.createClient("username", "password")

td.verify(client.setSecurity(credentials));

This did the trick :)

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

No branches or pull requests

3 participants