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.replace('mongoose') throws exception #316

Closed
gotenxds opened this issue Dec 23, 2017 · 5 comments
Closed

td.replace('mongoose') throws exception #316

gotenxds opened this issue Dec 23, 2017 · 5 comments

Comments

@gotenxds
Copy link

Hi, when trying to run td.replace('mongoose')

I get the following error:

     TypeError: Cannot read property 'scope' of undefined
      at Model.get [as id] (node_modules/mongoose/lib/services/document/compile.js:139:39)
      at copyObject (node_modules/lodash/_copyObject.js:29:24)
      at baseAssign (node_modules/lodash/_baseAssign.js:14:20)
      at baseClone (node_modules/lodash/_baseClone.js:120:32)
      at Object.clone (node_modules/lodash/clone.js:33:10)
      at exports.default (node_modules/testdouble/lib/imitate/create-imitation.js:32:29)
      at imitate (node_modules/testdouble/lib/imitate/index.js:31:46)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:38:83
      at newValue (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:39:20)
      at node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:18:28
      at node_modules/lodash/transform.js:60:12
      at node_modules/lodash/_createBaseFor.js:17:11
      at baseForOwn (node_modules/lodash/_baseForOwn.js:13:20)
      at Object.transform (node_modules/lodash/transform.js:59:39)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:14:52)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:37:29)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:38:83
      at newValue (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:39:20)
      at node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:24:16
      at node_modules/lodash/transform.js:60:12
      at node_modules/lodash/_createBaseFor.js:17:11
      at baseForOwn (node_modules/lodash/_baseForOwn.js:13:20)
      at Object.transform (node_modules/lodash/transform.js:59:39)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:14:52)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:37:29)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:38:83
      at newValue (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:39:20)
      at node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:24:16
      at node_modules/lodash/transform.js:60:12
      at node_modules/lodash/_createBaseFor.js:17:11
      at baseForOwn (node_modules/lodash/_baseForOwn.js:13:20)
      at Object.transform (node_modules/lodash/transform.js:59:39)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:14:52)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:37:29)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:34:26
      at arrayEach (node_modules/lodash/_arrayEach.js:15:9)
      at Object.forEach [as each] (node_modules/lodash/forEach.js:38:10)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:33:22)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:38:83
      at newValue (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:39:20)
      at node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:24:16
      at node_modules/lodash/transform.js:60:12
      at node_modules/lodash/_createBaseFor.js:17:11
      at baseForOwn (node_modules/lodash/_baseForOwn.js:13:20)
      at Object.transform (node_modules/lodash/transform.js:59:39)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:14:52)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:37:29)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:34:26
      at arrayEach (node_modules/lodash/_arrayEach.js:15:9)
      at Object.forEach [as each] (node_modules/lodash/forEach.js:38:10)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:33:22)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at node_modules/testdouble/lib/imitate/index.js:34:12
      at node_modules/testdouble/lib/imitate/overwrite-children/index.js:38:83
      at newValue (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:39:20)
      at node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:24:16
      at node_modules/lodash/transform.js:60:12
      at node_modules/lodash/_createBaseFor.js:17:11
      at baseForOwn (node_modules/lodash/_baseForOwn.js:13:20)
      at Object.transform (node_modules/lodash/transform.js:59:39)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/copy-props.js:14:52)
      at exports.default (node_modules/testdouble/lib/imitate/overwrite-children/index.js:37:29)
      at imitate (node_modules/testdouble/lib/imitate/index.js:33:35)
      at exports.default (node_modules/testdouble/lib/replace/module.js:12:41)
      at Object.exports.default [as replace] (node_modules/testdouble/lib/replace/index.js:9:29)
      at Context.beforeEach (test/middlewares/authMiddlewares.test.js:58:21)
@searls
Copy link
Member

searls commented Dec 24, 2017

We can't hope to provide support for successfully imitating every third party module under the sun (there are a lot of goofy ways to structure JavaScript that don't clone well), and in general we don't encourage folks to directly punch-out third party libraries (see: Don't Mock What You Don't Own).

That said, it would be a great help if you could try debugging this for us, both by looking at the part of mongoose that's exploding and potentially bisecting the failure against testdouble.js's repo to see if a particular commit introduced the break

@gotenxds
Copy link
Author

Hi, thanks for answering, I've read 'don't mock what you don't own' and I must say I disagree with the given solution of wrapping every third side library, this is just choose duplication in my opinion and even worse is writing code just to pass a test and not because it's giving us any value.
I will try to look into why this is exploding

@stephan-nordnes-eriksen
Copy link

stephan-nordnes-eriksen commented Jan 16, 2018

@searls;

The majority of my point below still stands, but;

I was mistaken; It appears as though you can do this;

let someDatabaseConstructor = td.replace('someDatabase')
let standIn = {}
td.when(someDatabaseConstructor(), {ignoreExtraArgs: true}).thenReturn(standIn)

Original post below;


I agree with the "Don't mock what you don't own" to some (large) degree, but what if you want to test that the code in your adapter/wrapper works without crashing, which might be the case in a dynamically typed language?

And what if you are like me and really want to see 100% line coverage? I would really have loved to see some easy way to do the following:

let SomeDatabase = require('someDatabase')
function myDatabaseAdapterWrapper(name, host, port){
  return new SomeDatabase({port: port, host: host, name: name})
}
// TestFile
let SomeDatabase = require('someDatabase')
let someDatabaseConstructor = td.replace(SomeDatabase, 'constructor')
let whateverTestObject = {}
td.when(someDatabaseConstructor({/*...*/})).thenReturn(whateverTestObject)
assert(myDatabaseAdapter('test','localhost', 1234) === whateverTestObject)

This test might at first seem naive, but remember, Javascript does not necessarily notify you of typos; Eg;

let SomeDatabase = require('someDatabase')
function myDatabaseAdapterWrapper(name, host, port){
  return new SomeDatabaise({port: port, host: host, name: name})
}

Thus, you really want to run the code once just to be extra sure.

@Schoonology
Copy link
Contributor

Addressing some of this feedback in reverse order:

Unit tests cannot be the only kind of tests we write. SAFE tests and ad hoc testing are unavoidable, and should cover the "run the code once just to be extra sure" case. While I understand and respect your desire to use unit tests for this, tests are complexity, too. Each test we write brings with it a cost, and we want to make sure there's a good return on that investment.

100% code coverage via unit testing runs afoul of that same razor: are all of these tests worth the additional complexity? What is the return on investment for these tests? What unit tests can I remove without lowering the confidence I have when making changes? What unit tests are redundant with SAFE tests or ad hoc testing?

This should lead me back to the original issue: td.replace('third-party'). As @searls mentioned, it is beyond the scope of testdouble.js to try and be able to replace every third party module. Don't mock what you don't own is not about "writing code to make the tests pass", it's about balancing the same test complexity and return on investment from above: the complexity of the test code (which includes testdouble.js, since it's part of your test code whether you wrote it or not) would drastically increase. On the other hand, even the smallest shim would greatly lower the complexity of test code required to deliver the value you expect from your unit tests.

[In the event that it's useful and desirable, consider explicit dependency injection (as opposed to implicit disambiguation via the require resolution rules): it can act as the "shim" in a situation like this (the DI container manages mocking and replacement) without needing to write a bespoke shim for each third-party dependency.]

@searls
Copy link
Member

searls commented Jan 16, 2018

There is one thing that I would add to @Schoonology's points above, which is that it's okay if you don't agree with us on this! There are countless "right" ways to write software, and we want to be as flexible as we can be without potentially throwing others off the trail (it's why we were hesitant to introduce td.replace('module-name') in the first place).

So, in the interest of resolving your issue without creating a bunch of new work for the library, I would encourage you to provide your own 'mongoose' shim, like so:

const mongoose = td.replace('mongoose', td.object(['get', 'post', 'etc']))

I don't know what methods you're concerned with, but this would likely cover your case without actually calling require on mongoose or attempting to imitate it

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

4 participants