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 doesn't work with Jest #331

Closed
twilczek opened this issue Feb 2, 2018 · 10 comments
Closed

td.replace doesn't work with Jest #331

twilczek opened this issue Feb 2, 2018 · 10 comments

Comments

@twilczek
Copy link

twilczek commented Feb 2, 2018

Description

td.replace doesn't work with jest. I created a small GIF showing the error:
http://g.recordit.co/DONLJhs7LD.gif

In the GIF you can see I am running the same test using Mocha and Jest.
Unfortunately td.replace fails to replace required function when I run the spec with Jest.

I have created the repo where you can reproduce the issue:
git clone https://github.com/twilczek/jestFailRepro.git
npm install
npm run test:jest (fails)
npm run test:mocha (works)

Environment

tested on node version 8.3.0 and 9.3.0

@lgandecki
Copy link
Contributor

This is the issue described here testdouble/quibble#23 , I think this is causes because jest „beautifies” the stack for nicer error messaging, which makes it impossible for quibble to use it’s hack to find the function caller. I half-hacked that and there were still problems so I’m not sure if that’s the only reason though.
I’m hoping to put more time into this, as it’s pretty blocking the use of testdoublejs in apps/libraries that don’t do dependency injection

@searls
Copy link
Member

searls commented Feb 2, 2018

Hey @lgandecki thanks for the simple explanation. I wonder if there's a way for us to pre-empt the replacement of printStackTrace() by grabbing a reference to the original function at require-time and then holding it, and using that instead?

@searls
Copy link
Member

searls commented Feb 10, 2018

I suspect this is related to the fact Jest spawns VMs for its tests.

Here's a Jest stack trace as observed in @twilczek's very handy example:

[ '/Users/justin/code/testdouble/quibble/lib/quibble.js',
      '/Users/justin/code/testdouble/quibble/lib/quibble.js',
      '/Users/justin/code/testdouble/testdouble.js/lib/replace/module.js',
      '/Users/justin/code/testdouble/testdouble.js/lib/replace/index.js',
      './tests/foo.test.js',
      '/Users/justin/code/twilczek/jestFailRepro/node_modules/jest-jasmine2/build/jasmine_async.js',
      '/Users/justin/code/twilczek/jestFailRepro/node_modules/jest-jasmine2/build/queue_runner.js',
      '/Users/justin/code/twilczek/jestFailRepro/node_modules/jest-jasmine2/build/queue_runner.js',
      '/Users/justin/code/twilczek/jestFailRepro/node_modules/jest-jasmine2/build/queue_runner.js' ]

And here's the same test run through Mocha:

[ '/Users/justin/code/testdouble/quibble/lib/quibble.js',
  '/Users/justin/code/testdouble/quibble/lib/quibble.js',
  '/Users/justin/code/testdouble/testdouble.js/lib/replace/module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'internal/module.js',
  '/Users/justin/code/testdouble/testdouble.js/lib/replace/index.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'internal/module.js',
  '/Users/justin/code/testdouble/testdouble.js/lib/index.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'internal/module.js',
  '/Users/justin/code/twilczek/jestFailRepro/tests/foo.test.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'internal/module.js',
  '/Users/justin/code/twilczek/jestFailRepro/node_modules/mocha/lib/mocha.js',
  '/Users/justin/code/twilczek/jestFailRepro/node_modules/mocha/lib/mocha.js',
  '/Users/justin/code/twilczek/jestFailRepro/node_modules/mocha/lib/mocha.js',
  '/Users/justin/code/twilczek/jestFailRepro/node_modules/mocha/bin/_mocha',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'module.js',
  'bootstrap_node.js',
  'bootstrap_node.js' ]

@searls
Copy link
Member

searls commented Feb 10, 2018

(This issue just became my war-room for documenting what seems to be going on, so feel free to mute notifications on it if you feel spammed)

This code is what we quibble relies on to implement td.replace, but if Jest runs every test in a VM, it's definitely not going to do us much good without rethinking the entire approach.

Oddly enough, node-sandboxed-module accomplishes require mocking by loading your subject under test in a VM, so that makes me suspect Jest's mocking facility does the same thing.

@searls
Copy link
Member

searls commented Feb 10, 2018

I suspect our best bet is to create a testdouble-jest plugin that uses jest-mock as a starting point to figure out how it's getting jest to replace which modules are loaded and then just calls the actual thing with our imitate() function and returns it.

I tried to read their code but had no luck understanding what it's doing.

@searls
Copy link
Member

searls commented Feb 10, 2018

I can also confirm that simply fixing the error trace is not sufficient to making quibble work with Jest. Other aspects of the way Jest runs tests are also preventing us from mocking require as you'd hope.

The following patch does not fix it for me:

diff --git a/lib/quibble.js b/lib/quibble.js
index 8607a8b..502c70f 100644
--- a/lib/quibble.js
+++ b/lib/quibble.js
@@ -26,8 +26,9 @@ var quibble
 module.exports = quibble = function (request, stub) {
   request = quibble.absolutify(request)
   Module._load = fakeLoad
+console.log('WAT', hackErrorStackToGetCallerFile())
   quibbles[request] = {
-    callerFile: hackErrorStackToGetCallerFile(),
+    callerFile: '/Users/justin/code/twilczek/jestFailRepro/tests/foo.test.js', //hackErrorStackToGetCallerFile(),
     stub: arguments.length < 2 ? config.defaultFakeCreator(request) : stub
   }
   return quibbles[request].stub
@@ -43,7 +44,7 @@ config = quibble.config()
 
 quibble.ignoreCallsFromThisFile = function (file) {
   if (file == null) {
-    file = hackErrorStackToGetCallerFile(false)
+    file = '/Users/justin/code/twilczek/jestFailRepro/tests/foo.test.js' // hackErrorStackToGetCallerFile(false)
   }
   ignoredCallerFiles = _.uniq(ignoredCallerFiles.concat(file))
 }
@@ -59,7 +60,7 @@ quibble.reset = function (hard) {
 
 quibble.absolutify = function (relativePath, parentFileName) {
   if (parentFileName == null) {
-    parentFileName = hackErrorStackToGetCallerFile()
+    parentFileName = '/Users/justin/code/twilczek/jestFailRepro/tests/foo.test.js' // hackErrorStackToGetCallerFile()
   }
   var absolutePath = absolutePathFor(relativePath, parentFileName)
   var resolvedPath = nodeResolve(absolutePath)
@@ -100,7 +101,7 @@ var stubbingThatMatchesRequest = function (request) {
 var requireWasCalledFromAFileThatHasQuibbledStuff = function () {
   const quibbleValues = _.values(quibbles)
   for (var i = 0; i < quibbleValues.length; i++) {
-    if (quibbleValues[i].callerFile === hackErrorStackToGetCallerFile()) {
+    if (quibbleValues[i].callerFile === '/Users/justin/code/twilczek/jestFailRepro/tests/foo.test.js' /*hackErrorStackToGetCallerFile()*/) {
       return true
     }
   }

@searls
Copy link
Member

searls commented Feb 10, 2018

Okay, job's done.

I just released testdouble-jest and testdouble@3.4.0. This should work now if you use td.mock() instead of jest.mock(). The public API is identical.

#337 should add a check to see if we're inside a jest run from the src/replace/module.js module and error out, warning users to use testdouble-jest instead.

@searls searls closed this as completed Feb 10, 2018
@lgandecki
Copy link
Contributor

Fantastic! Thanks @searls
I will give it a try later tonight.

@searls
Copy link
Member

searls commented Feb 11, 2018

Heads up that today I published testdouble-jest@1.0.1 and testdouble@3.5.0. In concert, setting up testdouble-jest should make td.replace just work™ without having to know about or call td.mock() directly

@lgandecki
Copy link
Contributor

Works like a charm, I've added some quick tests to jest-runner-standard (https://github.com/TheBrainFamily/jest-runner-standard/blob/bce5e28b6d958b7228baab1b6125ae63e736fec6/run.spec.js - relatively low value, but with the fantastic td syntax it was like 30 minutes total :) worth it.)

Meanwhile I realized that I could just use:

jest.mock('./helpers/getLocalStandard', () => ({getLocalStandard: td.func()}))
const { getLocalStandard } = require('./helpers/getLocalStandard')

but the consistency with other testing frameworks, and also the much cleaner/concise syntax is great, this:

const {getLocalStandard} = td.replace('./helpers/getLocalStandard', {getLocalStandard: td.func()})

hands down!

Thanks again :) It was a smart call to use the jest internals instead of trying to hack the quibble into submission.
and also thx @twilczek for the nice repro!

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