Classes with overridden methods not doubling properly since 1.10.0

[artlogic]

I've noticed a regression with 1.10.0. I have a number of custom error types that look something like this:

// Every built in error inherits from this error
error.BaseError = function () {
  var tmp = Error.apply(this, arguments);
  tmp.name = this.name = 'BaseError';

  this.message = tmp.message;
  this.status = 500;
  Error.captureStackTrace(this, this.constructor);
};
util.inherits(error.BaseError, Error);  // node's util module

error.BaseError.prototype.toJSON = function () {
  return {
    type: this.name,
    message: this.message
  };
};

// used when the user cannot access something
error.AccessDeniedError = function (message) {
  error.BaseError.call(this, message);
  this.name = 'AccessDeniedError';
  this.status = 403;
};
util.inherits(error.AccessDeniedError, error.BaseError);

Test double handles this perfectly! I can td.replace the module with these errors, and when AccessDeniedError is thrown in code I'm testing, I can verify the creation of the object and even test to see that it's been thrown (via chai's .throws). In essence, this returns true:

new errorHelper.AccessDeniedError('this is a test') instanceof errorHelper.AccessDeniedError

However, things stop working as of 1.10.0 when I override a method, like so:

// Used when the user passes in invalid data
error.ValidationError = function (message, validations) {
  error.BaseError.call(this, message);
  this.name = 'ValidationError';
  this.status = 422;

  if (typeof validations === 'undefined') {
    validations = null;
  }
  this.validations = validations;
};
util.inherits(error.ValidationError, error.BaseError);

error.ValidationError.prototype.toJSON = function () {
  var json = error.BaseError.prototype.toJSON.call(this);

  json.validations = this.validations;

  return json;
};

Now I'm getting TypeError: errorHelper.ValidationError is not a constructor when trying to create a new ValidationError after the module has been required via td.replace. AccessDeniedError continues to work.

[searls]

Thanks for the report! I'll try to look at this tomorrow

[searls]

Reading your issue, I don't understand how you're using td.replace or what you're testing for. When I read "test that something was created", I worry that you may not be verifying something that td is designed or intended to verify (namely, instantiation via a constructor method).

Can you please add an example in a branch to the examples/node project? Or to examples/babel if you're using necessary features?

[artlogic]

In particular, I have a module that has a number of exceptions (classes) and a few functions in it. I td.replace the module so I can mock the functions (an error handler / wrapper). It used to be that the mocked exceptions could still be thrown (and verified, via chaijs's .throws). Now, this is not the case for exceptions that override super class methods.

Given what you said, it sounds like this isn't a valid use case?

I'm happy to make an example if this doesn't clarify the issue.

[searls]

A-ha, I see now. The key of the issue is "module has a few classes on it", which I couldn't see in your description. In the future the only real way to discuss this stuff is with an example test & module, because I still may be reading you wrong.

But assuming I understand you, tes, 1.10 really screwed this use case up. Let me try to explain what's happening, then what you can do to workaround, then what should be done about it.

What was happening

Your module, call it errors.js, was having all of those Error constructor functions replaced with td.func() functions. That means if you were doing any instanceof checks you may not have really successfully verifying what you thought you were. If the subject was calling new on those, then I really have no clue what was happening because we'd never thought of instantiating td.js functions before with new. This is effectively an edge case that's loosely related to #107.

What's happening

As of 1.10, and in order to get named exports of instantiables to work, we now iterate over the top-level properties that are exported and do constructor-replacement on those as well, instead of simply doubling the functions we find. This is because export default class Foo is actually transpiled down to {default: Foo}).

Suppose errors looks like:

module.exports = { 
  Error1: class Error1 {
    complain() {}
  },
  Error2: class Error2 {
    complain() {}
  } 
}

So now, when your test says:

//test:
fakeErrors = td.replace('./errors')

//production:
realErrors = require('./errors')

fakeErrors will have properties named Error1, Error2, and so on, but these will just be bags of td.js double functions for each of their prototype functions and not a constructor function itself, because of how constructor-replacement has always worked (the test gets a pseudo-instance of the type, the subject gets a dummy constructor). That is to say, your test is getting this back:

{
  Error1: { complain: [td function] },
  Error2: { complain: [td function] }
}

Meanwhile, realErrors will have named Error1, Error2 properties as well, and these will be constructors such that the subject can instantiate them. Meaning calling require after you've replaced it will give the subject:

{
  Error1: TestDoubleConstructor([complain]),
  Error2: TestDoubleConstructor([complain])
}

Doing instanceof checks of Error1 and Error2 as a result are what led to somebody realizing #107 was a problem, since the constructor is a silly named function that td.js defines here.

Why this is happening

The reason test double is this way is because in most cases the test just wants to get at the test double functions but the subject needs to continue using new on whatever it's given. This turned out to be a naive design approach given the myriad ways people are wanting to use ES6 classes (see #164), and I think the most likely API-breaking change we will make is to unwind this.

Workaround for now

For now, you can workaround this in a couple ways:

1. Assert throws message instead of type
2. Assert throws type by re-requiring the './errors' in your test and then using the reference to Errors1 that gives you, but knowing that it may be insufficient b/c it'll possibly also pass instanceof checks with other TestDoubleConstructor types (Error2 for instance) -- in fact I have no idea if this is true; if you try this please let me know what you find
3. Require this dependency normally from your test instead of using td.replace on the whole module. Then, once you have a real errors module, calltd.replace(errors.Error1.prototype, 'complain') to replace just the function you need. This is not ideal because it'd make it a partial mock, but it wouldn't get in the way of your type comparisons
4. Don't fake these errors out. It's a little unusual to be replacing errors with test doubles in the first place, since they're most often used as Values and don't have any real behavior; I suspect that if I'd been pairing with you on these tests, I would have recommended we not have faked these errors out in the first place

Longer term plan

I'm coming around to the idea that this constructor-replacement scheme is going to need to become symmetrical between the test and the subject, which would be an API-breaking change. I think what this looks like:

1. Use something like require('util').inherit(TestDoubleConstructor, theTypeBeingDoubled) to try to resolve #107 and have faked constructors pass instanceof checks (I hope that works)
2. return a constructor to the test instead of a bag of test doubles, which would resolve issues like #164 immediately, but potentially make more pedestrian replacements (where the current behavior is preferable) more awkward. But at the very least, pass the test a reference to the constructor (like __constructor) on that bag.

[searls]

We're going to make this work, but it's going to require a major bump so it'll be a while.

Closing in favor of #166

[artlogic]

That's great to hear. Thank you for your thoughtful response. I'd be happy to provide an example if you think it might help move things forward.

[searls]

No, I'm almost sure I know what would fix you. Please read #166 to see if that seems right to you

[artlogic]

I'm sorry to keep responding, but I feel like there's a larger issue at play here. It's possible I've just misunderstood your posts, or what td is capable of. Here's a gist. I hope this clarifies: https://gist.github.com/artlogic/28026869470ac36e2fed4efc55d3e357

To me, it doesn't seem like td should break instantiation... but maybe I'm doing something wrong?

Thanks for your time.

[searls]

Yep, what you're experiencing is just how the 1.x series API is supposed to work: the test is returned a pseudo-instance and not a constructor when a constructor is replaced. What broke you in 1.10.x is that we caught that this doesn't apply to the top-level properties of a module, so we "fixed it". Of course, us fixing the glitch is what broke you and how you ended up here.

If you look at #166, we're proposing changing gears to instead return the same instantiable constructor to the test as the subject receives for the sake of obviousness and to reduce the number of edge cases created. That means we're working our way back to the way you were apparently using td.js previously, but via a more intentional route