Throw when moduleNameMapper points to inexistent module #3567

lindgr3n · 2017-05-14T18:22:17Z

Summary
Fixes #1888
Should get error and test fail if the module configured in moduleNameMapper is not found.

Right now if the module is not found the test still passes. Test should fail so user is aware that the configuration is wrong.

Test plan
Need to add two test.

One providing a module that exist
One providing a module that don't exist
Tried to get something to work but could need some pointers on how to mock out some parts. So left it out for now.

thymikee

Awesome! Can you also add a test to verify throwing error works?

thymikee · 2017-05-14T18:27:50Z

packages/jest-resolve/src/index.js

@@ -16,6 +16,8 @@ import type {ModuleMap} from 'types/HasteMap';
 const path = require('path');
 const nodeModulesPaths = require('resolve/lib/node-modules-paths');
 const isBuiltinModule = require('is-builtin-module');
+const chalk = require('chalk');


You'll need to add chalk to jest-resolve's dependencies.

thymikee · 2017-05-14T18:28:51Z

packages/jest-resolve/src/index.js

-            })
-          );
+            });
+          if(!module) {


You'll need to run npm run prettier to fix some styling issues.

lindgr3n · 2017-05-14T18:41:04Z

Commited prettier and chalk dependencies change.

lindgr3n · 2017-05-14T18:51:39Z

@thymikee Working on the test but not sure on how to implement the test.
My first attempt is something like

it('Should throw error when provided module that dont exist', () => {
const moduleMap = new ModuleMap();
const resolver = new Resolver(moduleMap, {});
expect(resolver.getMockModule('./', 'moduleThatDontExist')).toThrow();
});

But this get undefined error in ModuleMap. Not sure on what i should pass down to getMockModule so it reaches this._resolveStubModuleName(from, name); Any hint on where i could look?

thymikee · 2017-05-14T19:05:00Z

You need to feed Resolver with some data in constructor (e.g. moduleNameMapper option). If you find this test hard to do, you can always write an integration test.

lindgr3n · 2017-05-16T07:28:28Z

@thymikee Not sure if the integration test is in the correct place or correct written, but its a start.

thymikee

Nice! I've left some inline comments but it's heading in the right direction. Make sure to remove 'use strict' and @flow calls from test files.

thymikee · 2017-05-16T07:37:21Z

integration_tests/__tests__/modulenamemapper-test.js

+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @emails oncall+jsinfra


Please remove this

thymikee · 2017-05-16T07:38:06Z

integration_tests/__tests__/modulenamemapper-test.js

+ *
+ * @emails oncall+jsinfra
+ */
+'use strict';


Use strict is no longer necessary, babel transform adds this for us

thymikee · 2017-05-16T07:39:17Z

integration_tests/__tests__/modulenamemapper-test.js

+test('moduleNameMapper wrong configuration', () => {
+  const result = runJest('moduleNameMapper-wrong-config');
+  // Should fail because that wrong configuration will throw
+  expect(result.status).toBe(1);


Can you also catch a snapshot if this error? It's great that we know it fails, but even better if we see what's thrown

thymikee · 2017-05-16T07:41:28Z

integration_tests/moduleNameMapper-correct-config/__tests__/index.js

+
+test('moduleNameMapping correct configuration', () => {
+  importedFn();
+  expect(importedFn.mock.calls.length).toBe(1);


I'd rather like not to mock here, but meh, I guess it's fine

thymikee · 2017-05-16T07:41:53Z

integration_tests/moduleNameMapper-correct-config/package.json

+                "\\.(css|less)$": "./__mocks__/styleMock.js"
+        }
+    }
+  }


Add newline

thymikee · 2017-05-16T07:43:30Z

integration_tests/__tests__/modulenamemapper-test.js

@@ -0,0 +1,23 @@
+/**


Can we use camelCase name for this file?

thymikee · 2017-05-16T07:43:58Z

integration_tests/moduleNameMapper-correct-config/index.js

+ * LICENSE file in the root directory of this source tree. An additional grant
+ * of patent rights can be found in the PATENTS file in the same directory.
+ *
+ * @flow


Flow is not necessary here

lindgr3n · 2017-05-16T19:13:15Z

@thymikee Thx for the feedback! Think i managed to fix the things you pointed out.
Refactored to use snapshot test.
Removed mock in testfiles.
Unsure if you wanted the full copyright header to be removed in moduleNameMapper.test.js?

thymikee

One more round and after merging #3587 and we're good to go. Please rebase your branch to master.

thymikee · 2017-05-16T21:16:11Z

integration_tests/__tests__/__snapshots__/moduleNameMapper-test.js.snap

+      Configuration error:
+
+      Unknown module in configuration option moduleNameMapper
+      Please check: /\\\\.(css|less)$/: module-that-dont-exist


This double error is fixed here: #3587

thymikee · 2017-05-16T21:18:07Z

integration_tests/__tests__/moduleNameMapper-test.js

@@ -0,0 +1,18 @@
+const runJest = require('../runJest');


Please add copyright headers to every JS file. I mean just like this:

/** * Copyright (c) 2014-present, Facebook, Inc. All rights reserved. * * This source code is licensed under the BSD-style license found in the * LICENSE file in the root directory of this source tree. An additional grant * of patent rights can be found in the PATENTS file in the same directory. */

Without 'use strict'; (rebase to master to see why) without @flow (we're currently not type-checking our tests) and without @emails...

thymikee · 2017-05-16T21:18:22Z

integration_tests/moduleNameMapper-correct-config/index.js

+ */
+
+'use strict';
+// Inlcude style so moduleNameMapping triggers


This comment is unnecessary

thymikee · 2017-05-16T21:18:43Z

integration_tests/moduleNameMapper-correct-config/package.json

@@ -0,0 +1,7 @@
+{


Fix indentation, change to 2 per tab instead of 4.

thymikee · 2017-05-16T21:18:51Z

integration_tests/moduleNameMapper-wrong-config/index.js

+ */
+
+'use strict';
+// Inlcude style so moduleNameMapping triggers


Remove comment

thymikee · 2017-05-16T21:19:00Z

integration_tests/moduleNameMapper-wrong-config/package.json

@@ -0,0 +1,7 @@
+{


Fix indentation

thymikee · 2017-05-16T21:20:49Z

integration_tests/moduleNameMapper-wrong-config/package.json

+{
+    "jest": {
+        "moduleNameMapper": {
+                "\\.(css|less)$": "module-that-dont-exist"


"module-that-dont-exist" -> "no-such-module" maybe? Your name has grammar issues.

lindgr3n · 2017-05-16T21:51:20Z

@thymikee First time doing a rebase.
Is it enough to do "git rebase master" or do i need some more actions?

thymikee · 2017-05-16T21:54:17Z

When on your branch you should be good with:

git pull -r upstream master

Assuming upstream is the name of original Jest remote.
You can look it up with git remote -v.

lindgr3n · 2017-05-16T22:00:45Z

Looks like i messed that up somehow.
$ git remote -v
origin git@github.com:lindgr3n/jest.git (fetch)
origin git@github.com:lindgr3n/jest.git (push)
So guess i need to
$ git remote add upstream https://github.com/facebook/jest.git
Then i can go on with
$ git pull -r upstream master

thymikee · 2017-05-16T22:01:29Z

Exactly

lindgr3n · 2017-05-16T22:12:49Z

Just checking so im not heading in the wrong direction.

Your branch and 'origin/issue-1888' have diverged,
and have 13 and 6 different commits each, respectively.
(use "git pull" to merge the remote branch into yours)
nothing to commit, working tree clean

$ git pull
Merge made by the 'recursive' strategy.
$ git status

On branch issue-1888
Your branch is ahead of 'origin/issue-1888' by 14 commits.
(use "git push" to publish your local commits)
nothing to commit, working tree clean

Just do a git push?
The 14 commits are they the sum from master and from my branch?

thymikee · 2017-05-16T22:18:42Z

Meh, just push it then.

lindgr3n · 2017-05-16T22:21:06Z

Ty, back on track!

lindgr3n · 2017-05-16T22:45:01Z

Looks like #3587 didn't merge. Still got double Configuration in the snapshot.

lindgr3n · 2017-05-16T23:00:25Z

Ahh it haven't been merged yet :)

thymikee · 2017-05-17T09:20:18Z

Hey @lindgr3n, I've fixed some little nits and rebased your work on top of master properly, hope you don't mind. We still need to figure out why some errors are having duplicated messages, but it's definitely not in scope of this PR.

You'll be better off to hard reset to this branch to avoid local conflicts:

git reset --hard origin issue-1888

lindgr3n · 2017-05-17T09:28:43Z

Nice, no problem. Good work!
Need to learn how to rebase correct next time :)

lindgr3n · 2017-05-17T10:30:03Z

$ git reset --hard origin issue-1888
fatal: Cannot do hard reset with paths.
Did a
$ git reset --har issue-1888
But then i got into conflict state.
Guess it would be easier to just do a new checkout of the branch?

thymikee · 2017-05-17T10:31:14Z

git reset --hard origin/issue-1888 should fix that

lindgr3n · 2017-05-17T10:47:26Z

Ahh ty! Should have guessed that it should have been a / in the command.
Now it is correct!

lindgr3n · 2017-05-17T12:12:27Z

@thymikee Did i fix the changes you wanted or should i fix something more?

michahell · 2017-06-22T10:29:34Z

Could someone from JEST please look at this? A silently failing jest config is still something many newcomers to JEST are running into (many JEST ignore (s)css related SO posts etc.)

thymikee · 2017-06-22T10:33:48Z

There's an issue with this message being added twice, which we yet have to figure out: https://github.com/facebook/jest/pull/3567/files#diff-0454b3bf8e9e55c3655f3be2ff46cf28R19
I just didn't have time to look at this.

thymikee · 2017-06-27T20:30:56Z

@lindgr3n sorry about that, but I've fixed the branch for you.

However, you could definitely use some training in git rebasing.
This video series by @kentcdodds looks like a great way to start: https://egghead.io/lessons/javascript-how-to-rebase-a-git-pull-request-branch

I also fixed the message, it looks like this now:

Thanks for your work on that! Appreciated 👍

thymikee

Let's wait for CI to pass.

cpojer · 2017-06-28T06:30:31Z

CI is unhappy, needs prettier.

cpojer · 2017-06-28T06:44:15Z

Failing on windows.

lindgr3n · 2017-06-28T18:10:35Z

Thanks again @thymikee . Appreciate the help!

Checked the commits and i'm unsure why "integration_tests/tests/auto_clear_mocks.test.js" is changed. Feels like it should not have been changed?

Also the snapshots for "module_name_mapper.test.js" need to be updated.

thymikee · 2017-06-29T06:28:38Z

Skipping the test on Windows. Feature works, it's just the test output differs between UNIX and Windows. I'm not going to fix this now, as other tests may be skipped for the same reason.

lindgr3n · 2017-06-30T08:30:40Z

Big thanks @thymikee for helping me with my first contribution to OSS! :)
Will work on the rebase until the next time ;)

Keep up the good work!

* Fixes issue-1888 * Prettier fix and chalk to package.json dependencies * Added integration test * Added integration test * Refactored test to use snapshot * Refactored test to use snapshot * Rename to camelCase * Fixed lint warnings * Fixed lint warnings * Minor fixes * Use strict on integration tests * Added integration test * Refactored test to use snapshot * Fixed lint warnings * Added integration test * Refactored test to use snapshot * Minor fixes * Use strict on integration tests * Use Error instead of ValidationError to avoid duplicate message * Add fb copyright back to auto_reset_mocks test * Removed jest-validate dep from jest-resolve * Run prettier * Revert auto_clear_mocks change * Revert auto_reset_mocks change * Normalize test paths on windows * Don't use slash to normalize summary * Skip test on windows

github-actions · 2021-05-13T11:07:26Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label May 14, 2017

thymikee requested changes May 14, 2017

View reviewed changes

thymikee requested changes May 16, 2017

View reviewed changes

thymikee mentioned this pull request May 16, 2017

Empty stack for ValidateError #3587

Merged

thymikee requested changes May 16, 2017

View reviewed changes

thymikee force-pushed the issue-1888 branch from ec93885 to 922dec8 Compare May 17, 2017 08:03

thymikee approved these changes May 17, 2017

View reviewed changes

lindgr3n mentioned this pull request Jun 6, 2017

Errors in moduleNameMapper configuration seemingly ignored #1888

Closed

thymikee and others added 9 commits June 27, 2017 21:43

Use strict on integration tests

ee40b20

Added integration test

c8e99b0

Refactored test to use snapshot

48e965c

Fixed lint warnings

c2cefb8

Added integration test

c0ca429

Refactored test to use snapshot

f0238fc

Minor fixes

2d49788

Use strict on integration tests

3961af0

Use Error instead of ValidationError to avoid duplicate message

be6995f

thymikee force-pushed the issue-1888 branch from b9a18e5 to be6995f Compare June 27, 2017 20:26

thymikee added 2 commits June 27, 2017 22:33

Add fb copyright back to auto_reset_mocks test

0859f30

Removed jest-validate dep from jest-resolve

e5bd79d

thymikee approved these changes Jun 27, 2017

View reviewed changes

Run prettier

22f13ed

thymikee added 5 commits June 29, 2017 07:28

Revert auto_clear_mocks change

5f4b5b0

Revert auto_reset_mocks change

fe24f1a

Normalize test paths on windows

f6e5b14

Don't use slash to normalize summary

7f2ad5a

Skip test on windows

62952cd

thymikee changed the title ~~Fixes issue-1888~~ Throw when moduleNameMapper points to inexistent module Jun 29, 2017

cpojer merged commit 30f35c3 into jestjs:master Jun 29, 2017

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Throw when moduleNameMapper points to inexistent module #3567

Throw when moduleNameMapper points to inexistent module #3567

Conversation

lindgr3n commented May 14, 2017 • edited by thymikee Loading

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindgr3n commented May 14, 2017

lindgr3n commented May 14, 2017

thymikee commented May 14, 2017

lindgr3n commented May 16, 2017

thymikee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindgr3n commented May 16, 2017

thymikee left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindgr3n commented May 16, 2017

thymikee commented May 16, 2017

lindgr3n commented May 16, 2017

thymikee commented May 16, 2017

lindgr3n commented May 16, 2017

thymikee commented May 16, 2017

lindgr3n commented May 16, 2017

lindgr3n commented May 16, 2017

lindgr3n commented May 16, 2017

thymikee commented May 17, 2017 • edited Loading

lindgr3n commented May 17, 2017

lindgr3n commented May 17, 2017

thymikee commented May 17, 2017

lindgr3n commented May 17, 2017

lindgr3n commented May 17, 2017

michahell commented Jun 22, 2017

thymikee commented Jun 22, 2017

thymikee commented Jun 27, 2017

thymikee left a comment

Choose a reason for hiding this comment

cpojer commented Jun 28, 2017

cpojer commented Jun 28, 2017

lindgr3n commented Jun 28, 2017

thymikee commented Jun 29, 2017

lindgr3n commented Jun 30, 2017

github-actions bot commented May 13, 2021

lindgr3n commented May 14, 2017 •

edited by thymikee

Loading

thymikee left a comment •

edited

Loading

thymikee commented May 17, 2017 •

edited

Loading