[bug] duplicate manual mock found in separate directories

paularmstrong commented 7 years ago

Do you want to request a feature or report a bug? Bug

What is the current behavior?

Given a file tree:

src/app/modules
├── module1
│   ├── index.js
│   ├── __tests__/
├── module2
│   ├── index.js
│   ├── __tests__/

I use the modules outside of the modules directory by importing them by directory name:

import Module1 from '../modules/module1';
import Module2 from '../modules/module2';

I'd like to be able to mock module1 and module2. However, if I create src/app/modules/module1/__mocks__/index.js and src/app/modules/module2/__mocks__/index.js, I'm given the duplicate manual mock found error from jest-haste-map.

If, however, I try to create src/app/modules/__mocks__/{module1.js,module2.js}, the mocked files are not used.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal repository on GitHub that we can npm install and npm test.

See above behavior.

What is the expected behavior?

I would expect either approach to work, given that the first case uses different paths and the second case uses the pathname of the module.

Run Jest again with --debug and provide the full configuration it prints. Please mention your node and npm version and operating system.

node v6.2.0 npm v3.8.9 OS X 10.11.6

> NODE_ENV=test jest --env jsdom "--debug" "src/app/redux/modules/devices"

jest version = 17.0.0
test framework = jasmine2
config = {
  "moduleFileExtensions": [
    "js",
    "json"
  ],
  "moduleDirectories": [
    "node_modules"
  ],
  "moduleNameMapper": [
    [
      "^.+\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$",
      "/Users/paul/dev/tools/jest/mock-assets.js"
    ],
    [
      "^.+\\.css$",
      "identity-obj-proxy"
    ]
  ],
  "name": "dev",
  "setupTestFrameworkScriptFile": "/Users/paul/dev/tools/jest/setup-framework.js",
  "testPathDirs": [
    "/Users/paul/dev/src"
  ],
  "testRegex": "/__tests__/.*\\.test\\.js$",
  "timers": "fake",
  "rootDir": "/Users/paul/dev",
  "setupFiles": [],
  "testRunner": "/Users/paul/dev/node_modules/jest-jasmine2/build/index.js",
  "testEnvironment": "/Users/paul/dev/node_modules/jest-environment-jsdom/build/index.js",
  "transform": [
    [
      "^.+\\.jsx?$",
      "/Users/paul/dev/node_modules/babel-jest/build/index.js"
    ]
  ],
  "usesBabelJest": true,
  "automock": false,
  "bail": false,
  "browser": false,
  "cacheDirectory": "/var/folders/dm/vt920lmd6tzdq_709zkykwx40000gn/T/jest",
  "coveragePathIgnorePatterns": [
    "/node_modules/"
  ],
  "coverageReporters": [
    "json",
    "text",
    "lcov",
    "clover"
  ],
  "expand": false,
  "globals": {},
  "haste": {
    "providesModuleNodeModules": []
  },
  "mocksPattern": "__mocks__",
  "modulePathIgnorePatterns": [],
  "noStackTrace": false,
  "notify": false,
  "preset": null,
  "resetMocks": false,
  "resetModules": false,
  "snapshotSerializers": [],
  "testPathIgnorePatterns": [
    "/node_modules/"
  ],
  "testURL": "about:blank",
  "transformIgnorePatterns": [
    "/node_modules/"
  ],
  "useStderr": false,
  "verbose": null,
  "watch": false,
  "cache": true,
  "watchman": true,
  "testcheckOptions": {
    "times": 100,
    "maxSize": 200
  }
}
jest-haste-map: duplicate manual mock found:
  Module name: index
  Duplicate Mock path: /Users/paul/dev/src/app/modules/push-notification-manager/__mocks__/index.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/Users/paul/dev/src/app/modules/push-notification-manager/__mocks__/index.js
 Please delete one of the following two files:
 /Users/paul/dev/src/app/modules/image-file/__mocks__/index.js
/Users/paul/dev/src/app/modules/push-notification-manager/__mocks__/index.js

No tests found
  1 file checked.
  testPathDirs: /Users/paul/dev/src - 1 match
  testRegex: /__tests__/.*\.test\.js$ - 0 matches
  testPathIgnorePatterns: /node_modules/ - 1 match

yongdamsh commented 7 years ago

+1

yongdamsh commented 7 years ago

In my case, after clear the cacheDirectory/var/folders/dm/vt920lmd6tzdq_709zkykwx40000gn/T/jest and re-install npm dependencies, these messages have been disappeared.

LegNeato commented 7 years ago

Here's the offending code:

https://github.com/facebook/jest/blob/cd8976ec50dbed79cfe07f275052cdd80d466e73/packages/jest-haste-map/src/index.js#L98

But it looks like the behavior might be explicitly wanted as there is a test confirming it:

https://github.com/facebook/jest/blob/8de90b320c87a0a36d68f6bd8177620a985df269/packages/jest-haste-map/src/__tests__/__snapshots__/index-test.js.snap#L15

Which was added in:

https://github.com/facebook/jest/commit/cfade282fbbe2737b6dd2cee1cf3da3ee8624512

I wonder why we are using basename rather than the whole path as the key?

/cc @flarnie

LegNeato commented 7 years ago

This means that basenames for modules need to be globally unique in a project when using manual mocks. For my use case, it means I can't do something like:

import { MyWhatever } from 'models/MyWhatever/schema';
import { MyOtherWhatever } from 'models/MyOtherWhatever/schema';

and use manual mocks at the same time. Jest will currently see them both as mocking schema and complain.

Though the workaround is trivial (s/schema/MyWhateverSchema/), it feels like a bug to rename and restructure non-test code to make jest happy 🐞 .

cpojer commented 7 years ago

Yes this does suck indeed. The manual mocking system is really not good and I'm happy to accept PRs that will improve the situation, assuming we can make sure we don't break all of FB (but I can take care of that :) )

LegNeato commented 7 years ago

Cool. I might find some time tomorrow to cook up a patch, but no promises though 😅

juliankrispel commented 7 years ago

@cpojer is there a particular reason for this behaviour?

Could it have anything to do with the fact that fb uses unique filenames for modules? I don't see otherwise how not allowing two mocks with the same name makes any sense...

cpojer commented 7 years ago

Yes, mocks are "global" as well. This is a terrible design that we have to live with, unfortunately. At FB we have 4000+ mock files in the wrong location (and often there isn't even a proper location). It is likely we will fix this early next half, so this should improve in Jest. I'm happy to support PRs that improve the behavior in Jest for open source – if we can retain the old behavior for Jest at FB for now.

juliankrispel commented 7 years ago

@cpojer how about a flag? Would you accept a pr that enables/disables this with a flag?

cpojer commented 7 years ago

Yeah, it should be a config option. But I'm not just talking about the warning, I'm also talking about the feature in general.

juliankrispel commented 7 years ago

@cpojer right - which parts of jest does this touch?

cpojer commented 7 years ago

The resolution code is called from jest-runtime: https://github.com/facebook/jest/blob/master/packages/jest-runtime/src/index.js and is somewhere in jest-resolve and jest-resolve-dependencies.

juliankrispel commented 7 years ago

@cpojer thanks for the pointers :+1:

ColCh commented 7 years ago

@cpojer what about some global override, like JEST_USE_BASENAME_FOR_CACHING, to switch this behaviour?

At least, we can enjoy non-unique filenames, and it will not break anything in FB.

Of course, it's a temporary solution.

I mean, this is in some /etc/profile or ~/.bashrc

export JEST_USE_BASENAME_FOR_CACHING="true"

(or some file with env) and then

$ jest

or this one, without env file modifications:

$ JEST_USE_BASENAME_FOR_CACHING="true" jest

What you think? It's a sort of hack or it's ok? :wink:

EnoahNetzach commented 7 years ago

I just tried with a new repo, using two versions of jest (^15.0.0 and ^17.0.0) and, although the latter gives the warning, the test behaves as expected.

@ColCh I don't think the issue here is with the cache, probably a more suitable name could be JEST_USE_BASENAME_FOR_MOCKING.

EnoahNetzach commented 7 years ago

@cpojer if FB code has the restriction on the uniqueness of the names, using the full path as a key for the map of mocks shouldn't bring problems.

Am I right or there is something I'm not seeing?

EnoahNetzach commented 7 years ago

The two solutions I see are:

modify getMockName to accomodate the option to use the basename or the full path
remove that function altogether

ColCh commented 7 years ago

it would be nice to see @cpojer answer on it

cpojer commented 7 years ago

Hey everyone, sorry for the delay, I'm pretty backed up with a ton of stuff right now.

I think I'm fine if you guys decide to do whatever breaking change in Jest that is necessary to improve this system. Manual mocking is really messed up and doesn't work well. One thing we kind of want to do is limit the scope of "haste modules" (internal FB module system) using a config option, like "haste_modules": ['path/a', 'path/b']" and then only look at haste modules in those folders, including this weird mocking behavior. If anybody wants to make this change, that would be amazing.

One thing to be figured out then, is this: If all manual mocks are local, like __mocks__/a.js maps to a.js, what do we do with node_module mocks? For this there are a couple of ways:

Introduce a new __node_modules_mocks__ folder but that is ugly.
The top level __mocks__ folder as seen from rootDir (project root) could act as a global folder.

So to summarize:

Let's fix manual mocks in Jest!
Let's namespace haste modules and limit them to certain folders/regex whatever.
Make sure that the current mocking behavior still works for FB, even if that means we have to whitelist certain folders to make haste work (it would just look like ["<rootDir>"] for us for now, I guess)
Figure out how to still be able to mock node modules.

What do you think?

EnoahNetzach commented 7 years ago

In order:

Let's fix manual mocks in Jest!

HURRAY :smile: :tada:

Let's namespace haste modules and limit them to certain folders/regex whatever.
Make sure that the current mocking behavior still works for FB, even if that means we have to whitelist certain folders to make haste work (it would just look like [""] for us for now, I guess)

I'm not sure I understand the needs with haste. What you mean is to give the possibility to say "those modules are haste modules"? If we have four modules: /path_1/a, /path_1/b, /path_2/a, /path_2/c, and the setting is

"haste_modules:" ["/path_1/a", "/path_2/c"]

only /path_1/a and /path_1/b are restricted to exist only in /path_1, so /path_2/c is valid, and /path_2/a raises an error/warning.

I'd say, targets could easily be specific files and entire directories, even with single/double *.

Figure out how to still be able to mock node modules.

I'd maintain the current behavior:

If the module you are mocking is a node module (eg: fs), the mock should be placed in the same parent directory as the node_modules folder.

ColCh commented 7 years ago

My thoughts:

haste modules:

I think , haste_modules is nice, it fits just like collectCoverageFrom and other options: array of globs In case, if you have all src as haste modules, and just one directory is non-haste:

haste_modules: [
  "src",
  "!src/foo"
]

node_modules

@EnoahNetzach what if someone have same names for app module and module from node_modules?

To make it work without haste... hm, I think it can be described like:

given node module project/node_modules/react, mock will be inside of project/__node_modules_mocks__/react.js if you have a file project/react.js, then use project/__mocks__/react.js

(of course, react.js is an example. here can be any filename among of all modules, which can be installed from npm)

Really, mocking node_modules module is a rare case by my experience, so... may be rareness may compensate ugliness in particular case of mocking node_modules ?

Anyone mocking modules inside of node_modules often?

what about to think further

as I noticed, for react-native projects, we often have to mock application modules and leave modules from node_modules unmocked (e.g. lodash)

this means, we have:

manual created mock component for each dumb component (dumb component are layout components)
a long list of jest.mock call in every test file

what I want to say: it would be very nice to have ability to auto mock modules on some paths.

It can be implemented around well-known data structure array-of-jest-globs in config, and filtering modules upon it.

I will describe that step by step

Given this config entry

"autoMockingPaths": [
  "src/components/dumb/**/*.js",
]

and this code at src/screens/app.js:

import _ from 'lodash';
import Button from '../../components/dumb/button.js';

// blah blah AppScreen implementation skipped

and this test code for screen at src/screens/__tests__/app-test.js:

import AppScreen from '../app.js';

describe('AppScreen', () => /* testing app screen */);

We come to this situation, in context of app-test.js:

AppScreen is not mocked
lodash is not mocked
Button, which is required by AppScreen, is mocked

... You can answer, how it will play with automock config entry?

simply saying, automock: true is equivalent to:

"autoMockingPaths": [
  "<rootDir>"
]

auto mocking... wait!

may be just introduce special value for automock? at least it will not break people's configs

for example, with this config entry:

automock: "app"

jest will auto mock all application modules, and leave actual versions for modules from node_modules

what do you think about app level modules automocking, @cpojer ? I find it very efficient for my particular case.

EnoahNetzach commented 7 years ago

I fully agree with "haste_modules".

We personally don't use automocking that much, so I can't say what's better, my wild guess is that the "autoMockingPaths" var could be useful and elastic enough. On the contrary I find "automock": "app" too stiff (jest already disabled automocking by default).

The __node_modules_mocks__ could be an option, I agree that rareness compensate for ugliness (in my particular case, we rarely mock node_modules, and when we have to do it, we use jest.mock(...)). The only caveat is: what happens when you have a nested node_modules folder (e.g. src/node_modules), do you have to mock its modules from the global __node_modules_mocks__, a nested version of it, or normally with __mocks__ co-located?

ColCh commented 7 years ago

may be just throw, if someone has same module name in node_modules and app modules

e.g.

app/express.js as app module (may be I'm doing train game) and app/node_modules/express as web server from npm throw new Error("can't mock express.js file - it duplicates one from node_modules")

in this case, __mocks__ can be used for node_modules, but dev have to rename own modules on such collisions

nah... this is more ugly than __node_modules_mocks__, isn't it?

EnoahNetzach commented 7 years ago

What I meant is: what if you have an npm installed module x and later, deeper in your codebase define a module x in a nested node_modules folder?

The naming collision is usually handled in node by preferring the nearest, but I don't know how this works in haste.

I'm bringing this up because projects like Create React App are using it, or will do in the near future.

On a side note, @cpojer is this issue somehow related?

cpojer commented 7 years ago

Let's keep this focused on changing how haste works (whitelist/blacklist rather than on by default). I do think I'd prefer to maintain that <rootDir>/__mocks__ should be the default for node module mocks. We could make this a configuration option as well: "globalMocks" that defaults to <rootDir>/__mocks__. Is anybody willing to work on this?

EnoahNetzach commented 7 years ago

I should be able to work on this no sooner than the next weekend.

ColCh commented 7 years ago

I can work on PR this sunday, I'm kinda free

@cpojer just to recap - create globalMocks config entry with default value of <rootDir>/__mocks__. This option regulates use of node-haste within jest by specifying path? Or it will be array of paths?

cpojer commented 7 years ago

I think these are some larger changes on the way to get this resolved but I think we need both the singular globalMocks option (could be a string or array of strings) and the hasteModules option which would be an array of paths of haste modules. Most of this code lives in jest-haste-map and jest-resolve. I'm not 100% sure what the right solution will look like yet.

From: Max Sysoev notifications@github.com Sent: Friday, December 9, 2016 8:18:44 AM To: facebook/jest Cc: Christoph Pojer; Mention Subject: Re: [facebook/jest] [bug] duplicate manual mock found in separate directories (#2070)

I can work on PR this sunday, I'm kinda free

@cpojerhttps://github.com/cpojer just to recap - create globalMocks config entry with default value of /mocks. This option regulates use of node-haste within jest by specifying path? Or it will be array of paths?

- You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/facebook/jest/issues/2070#issuecomment-265958606, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAA0KAMFc34iKqBDLHZzgaGHqyc3WkAzks5rGQ7kgaJpZM4Kt2DW.

ColCh commented 7 years ago

Sorry, my workstation became broken, and apparently no way to get it working in couple of months (1-2 months). I even lost my project on this PR :( So, sorry for taking responsibilities.

mwildehahn commented 7 years ago

~~What about just a config option to change the behavior of getMockName?~~

~~Not too familiar with the internals of jest, but it looks like that is the simplest solution to fix the issue without breaking jest for FB.~~

This is going to be more complicated than I originally thought. To me, manual mocks should replace the file they're closest too, ie. something like this:

{ 'aws-sdk': '/Users/project/__mocks__/aws-sdk.js',
  'slack': '/Users/project/__mocks__/slack.js',
  '/Users/project/db/index': '/Users/project/db/__mocks__/index.js',
  '/Users/project/slack/index': '/Users/projects/slack/__mocks__/index.js' }

require('aws-sdk') should resolve to the /Users/project/__mocks__/aws-sdk.js this is a mock for a node_module.

require('./db') (or any path to db) should resolve to: /Users/project/db/__mocks__/index.js.

My understanding of the way to setup jest manual mocks (and there should probably be more documentation if something like the above is used) was that they should be as close to the file being mocked as possible within a __mocks__ directory.

Manual mocks are defined by writing a module in a mocks/ subdirectory immediately adjacent to the module. (https://facebook.github.io/jest/docs/manual-mocks.html)

Given that, the above behavior makes the most sense to me.

Thoughts?

mwildehahn commented 7 years ago

This is an initial pass at implementing something like the above: https://github.com/facebook/jest/compare/master...mhahn:issue-2070-new-mock-resolution

It breaks the majority of the jest unit tests because it no longer allows for pure "named" mocks like the following: https://github.com/facebook/jest/blob/master/packages/jest-runtime/src/__mocks__/createRuntime.js

IMO, something like createRuntime is a test utility that should be in a test utils folder and imported in the tests that need it. The way it is implemented as a manual mock doesn't really make sense to me as something that should be preserved.

One option is to add a config variable that will switch between the existing behavior and cleaned up version of the changes above. I'm not really sure what that should be called though.

cpojer commented 7 years ago

We cannot break the current behavior as we rely on it extremely heavily at Facebook.

mwildehahn commented 7 years ago

@cpojer i agree. i didn't really understand this comment after reading through the code: https://github.com/facebook/jest/issues/2070#issuecomment-265027510. maybe we could support whitelisting a list of directories that will resolve to the current behavior?

mwildehahn commented 7 years ago

what about two new config options:

fullPathMockResolution (defaulted to false to keep existing behavior) coupled with:
namedMockDirectories: if fullPathMockResolution is enabled, any directories within that array will be resolved with the existing behavior. For the FB use case, this would just be [<rootDir>] like you mention above.

This lets devs opt into the full path resolution so it doesn't require changes by existing jest installs and also enables the existing behavior for specific directories if they want that.

cpojer commented 7 years ago

cc @voideanvalue this may be something you'll have to think about (namespace manual mocks as part of the new haste implementation).

joaoreynolds commented 7 years ago

+1, is there a way to fix this?

paranoidjk commented 7 years ago

+1

dkundel commented 7 years ago

Any updates on how to fix this problem? This turns out to be really painful if you follow a structure like this:

project/
├── models
│   ├── index.js
│   ├── __mocks__/
│   │   ├── index.js/
├── challenges
│   ├── index.js
│   ├── __mocks__/
│   │   ├── index.js/

Right now I have to manually mock a module in the test if I'm using two with a 'conflicting' name even though they actually don't have conflicting names since the path should be considered.

Cheers, Dominik

devlato commented 7 years ago

@cpojer Any updates here guys? Is there a workaround?

simonhoss commented 7 years ago

A little workaround for me is to mock the modules with require

jest.mock('models/index', () => require('models/index/_mocks_/index'));

I renamed the __mocks__ folder name to _mocks_ so that jest does not catch the files.

One day when this will work I will rename _mocks_ to __mocks__ and remove the require part from jest.mock

hally9k commented 7 years ago

Firstly: Thanks for all the awesome work. Secondly: This is really, quite frustrating. I find myself forced to use jest.mock in the test file when the mocked file shares a name with any other file in the entire codebase that also gets mocked. The hoisting disallows the actual mock to be imported too, so it forces you to duplicate the mock in any two tests that require that file to be mocked, adding to the fragility of the test suite. Are we looking at addressing this? Does FB use this stuff? If yes, then how? 😞

slecorvaisier commented 7 years ago

+1 very frustrating to see these warnings. Can't wait for a fix.

cwmoo740 commented 7 years ago

I think the issue I'm having is related:

if I have a jest.mock('src/utils/history'), it also incorrectly mocks the 'history' node_module.

https://github.com/cwmoo740/jest-manual-mocks-node-modules

masoudkazemi commented 7 years ago

Guys any update?

brunolm commented 7 years ago

@masoudcs I think this got rid of the warning:

package.json

"jest": {
  /* other settings ... */
  "modulePathIgnorePatterns": ["<rootDir>/node_modules/react-native/Libraries/Core/__mocks__"]
}

Add the "duplicate" folders in the modulePathIgnorePatterns array and the warning will be gone

masoudkazemi commented 7 years ago

Thanks @brunolm

So it is just a warning, correct?! I just want to make sure this would not cause something bad happen, i.e. mocking index.js in multiple directories: src/app/modules/module1/__mocks__/index.js and src/app/modules/module2/__mocks__/index.js

brunolm commented 7 years ago

I don't know if it matters, but when I was running it was saying it would pick only the other one and this that I listed would be ignored either way.

So I think it is fine to tell it to ignore, it wasn't using it anyway.

masoudkazemi commented 7 years ago

Yes just like you said, and we did not face any issues till now. Hopefully, it is doing what it has to do! :) Thanks

fernandopasik commented 6 years ago

I tried what this last two commits suggested but didn't remove the warning.

jest.mock('dir/index', () => require('dir/__mocks_/index'));

adamhenson commented 6 years ago

I'm seeing this warning because I use GitBook (./_book directory), even though I have testPathIgnorePatterns: ['/_book/', ...otherStuff] in my configuration. Skimming through this issue, I don't see a clear cut workaround. I just want to stop seeing the warnings - any help would be appreciated.

jestjs / jest