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

Fix jest-preset to use internal preprocessor (fix testing with 0.56) #20068

Closed
wants to merge 2 commits into from

Conversation

vovkasm
Copy link
Contributor

@vovkasm vovkasm commented Jul 6, 2018

Fixes #19859 at least until problem will be fixed properly.

By unknown reason babel-jest can't transform tests and react-native's files, see #19859 for details. But internal preprocessor.js can, so use it for now in published jest-preset.json.

Test Plan:

From original issue:

  • Create app react-native init Test56 --version=0.56.0
  • cd Test56
  • mkdir __tests__
  • create file __tests__/AppTest.js that has following lines:
import React from 'react';
import renderer from 'react-test-renderer';
import App from '../App';

test('App matches Snapshot', () => {
  const component = renderer.create(<App />);
  let tree = component.toJSON();
  expect(tree).toMatchSnapshot();
});
  • npm test

Slighly modified repo for reproduction: https://github.com/dlowder-salesforce/rn-56-test/tree/eb0611b1870c410cd4a7458a740a3ac52bad2827
Fixed repo: https://github.com/vovkasm/rn-56-test/tree/a00fc639bd6a681591131c24e3ee976ae374438e

@kelset You want to be mentioned ;-)

Release Notes:

[GENERAL] [BUGFIX] [jest-preset.json] - Fix js transformation in jest

…est (should fix facebook#19859)

By unknown reason babel-jest can't transform tests and react-native's files, see facebook#19859 for details. But internal preprocessor.js can, so use it for now in published jest-preset.json.
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 6, 2018
@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 6, 2018

I think that failed end_to_end tests does not have any relation to this change... As I know, jest tests in generated application from template currently does not exists. See this comment: #19859 (comment) from @hramos and test plan above it.

@kelset
Copy link
Contributor

kelset commented Jul 6, 2018

Hey thanks for the PR! Yes it looks like all those tests are failing because of the packager not starting (?).

I'll let the core know, hopefully we can get this passing tests -> reviewed -> merged in master -> cherry picked for 0.56.1

@stevehollaar
Copy link

@vovkasm For me this workaround seems to break tests that rely on jest.mock manual mocks - presumably because babel-jest is no longer hoisting them above imports

@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 6, 2018

@stevehollaar Seems like that, mocks was didn't in test project, so I miss this check.
I see, jest/preprocessor.js has no any refs to babel-plugin-jest-hoist plugin.
I thinking about adding it to .babelrc, but it seems that RN's transformer hardcode BABEL_ENV: https://github.com/facebook/metro/blob/master/packages/metro/src/reactNativeTransformer.js#L161, so it will not be possible to optionally add this plugin only to test environment.

Do you have any suggestions?
Can you fork https://github.com/vovkasm/rn-56-test/ and provide failed test case? Then I can try to do something.

@stevehollaar
Copy link

Sadly I don't have any suggestions on where to go from here, but I added manual mocking to the test repo here: https://github.com/stevehollaar/rn-56-test, so the tests now fail there, since manual mocks are not hoisted. If you're able to get those tests green, that would be wonderful.

@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 7, 2018

Ok. I trying to walk another way and stay with babel-jest (because it automatically adds hoisting plugin). And found that babel-jest simple ignores project's .babelrc file, how and why is that? Answer is probably simple... it does not contains code that bring in content of .babelrc files into transform options :-/
See: here is function getBabelRC it is find and reads user's .babelrc files. But it's used only for calculation of hashes for cache-validity checks here.

Then I open my node_modules/babel-jest/build/index.js and add this chunk of code to funciton process:

--- SomeProjectWithUnmodifiedModules/node_modules/babel-jest/build/index.js	2018-06-25 17:01:32.000000000 +0300
+++ node_modules/babel-jest/build/index.js	2018-07-07 18:51:33.000000000 +0300
@@ -160,7 +160,16 @@
         return src;
       }

-      const theseOptions = Object.assign({filename}, options);
+      const babelRC = JSON.parse(getBabelRC(filename))
+      const theseOptions = Object.assign({filename}, babelRC);
+      for (const param in options) {
+        if (Array.isArray(theseOptions[param])) {
+          theseOptions[param] = options[param].concat(theseOptions[param])
+        } else {
+          theseOptions[param] = options[param]
+        }
+      }
+
       if (transformOptions && transformOptions.instrument) {
         theseOptions.auxiliaryCommentBefore = ' istanbul ignore next ';
         // Copied from jest-runtime transform.js

Now all tests pass as expected.
I think that it's all related to these jest bugs: jestjs/jest#6573, jestjs/jest#6617 - but actual situation is very simple - babel-jest doesn't use any options from you .babelrc

So:

  1. We can't use react-native/jest/preprocessor.js because it currently can't be directly configured to use babel-plugin-jest-hoist.
  2. We can't use babel-jest because it currently can't transpile RN code, because it currently can't configures babel with user-provided .babelrc

Any suggestions what to do welcomed!

@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 7, 2018

After some thinking. We can still apply this patch as a fast workaround (may be even update internal transformer to include hoisting of mocks). In the more far perspective someone should fix jest.

What do people think? What the intention of core team?

It will allow using jest mocks within tests configured with react-native
jest preset..
@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 7, 2018

After tracking history of jest/preprocessor.js until autumn 2016, I decided to add babel-plugin-jest-hoist into it. This script should only be used from jest which means that code processed for testing purposes which means we can safely add testing-only babel plugins.

@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 7, 2018

@stevehollaar Can you please test? Quick check can be done if you download jest/preprocessor.js from the PR's branch and place it by hand into node_modules/react-native/jest/preprocessor.js.

@sync

This comment has been minimized.

@stevehollaar
Copy link

@vovkasm Sorry for the delay, but yes! I can confirm that your updated preprocessor.js fixes the tests (both on the test repo: https://github.com/stevehollaar/rn-56-test, and on a real project)

@lehni
Copy link

lehni commented Jul 16, 2018

@vovkasm did you try and see if using https://github.com/lehni/babel-jest-nested fixes your issue?

And if it does, could you maybe add your voice to jestjs/jest#6617 to get some traction?

@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 16, 2018

@lehni To be clear... Personally I do not bitten by this error. We use typescript and ts-jest seems work properly (also we do not use jest mocks). But I will look and vote if solution is appropriate )

@kelset
Copy link
Contributor

kelset commented Jul 20, 2018

Hey @vovkasm just wanted to let you know that I'm trying to get this merged asap but we have been really busy trying to fix RN 0.56 working with Windows. We should be close to a fix for that so hopefully we'll be able to come back to this 💪

@evanjmg
Copy link

evanjmg commented Jul 22, 2018

so until this is merged, the only workaround is adding this to jest config? React native init creates a broken test suite, should probably merge this asap

"transform": {
      "^.+\\.js$": "<rootDir>/node_modules/react-native/jest/preprocessor.js"
    },

UPDATE: I had to copy the preprocessor file and add the mock support to it as seen in the PR. Thanks @vovkasm

@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 22, 2018

@evanjmg Yes, also you can write your own preprocessor (especially if your project uses custom babel configuration).

@kelset
Copy link
Contributor

kelset commented Jul 23, 2018

👋 everyone, since I couldn't find anyone else to review this PR I'm trying to investigate it deeply in order to be able to review it myself.

And I think that sadly this workaround should not be merged, since the issue is actually babel-jest not working correctly with babel7. The approach presented by the PR of using the preprocessor sadly locks users to not using custom .babelrc which IMHO is a no-go (ex. I need a different babel plugin from the one @vovkasm added to the config, I would need to do another PR to it in order to have it)

So tbh I'd prefer to understand and fix why babel-jest is not detecting the .babelrc config as it should.

I'll report back here once I have more info/ better ideas. For now I don't want to close this PR as its the only viable workaround.

@kelset kelset mentioned this pull request Jul 23, 2018
3 tasks
@vovkasm
Copy link
Contributor Author

vovkasm commented Jul 23, 2018

@kelset

  1. See my comment Fix jest-preset to use internal preprocessor (fix testing with 0.56) #20068 (comment), it contains partial analysis of babel-jest behavior.
  2. Imho, after fixing this bug, we should remove RN's own jest/preprocessor.js at all. And switch it to use babel-jest to detect such bugs early.

@kelset
Copy link
Contributor

kelset commented Jul 23, 2018

Yeah I was using your comments as guidance, thank you so much for your investigations :)

And I agree with the preprocessor, not sure if it's there because of some metro requirement but I'll ask around.

@kelset
Copy link
Contributor

kelset commented Jul 25, 2018

Hey @vovkasm I'm closing this for the reasons I mentioned earlier - but I wrote a "more complete" guide to fixing this issue over there #19859 (comment)

Thanks again for all your help!

@deecewan
Copy link
Contributor

deecewan commented Aug 17, 2018

fwiw, I just tried this fix out - it seems to not be working (anymore?).

this is what my devdeps looks like:

    "@babel/cli": "7.0.0-beta.47",
    "@babel/core": "7.0.0-beta.47",
    "@babel/parser": "^7.0.0-beta.48", // there is no beta.47
	"babel-core": "7.0.0-bridge.0",
	"babel-jest": "^23.4.2", // i'm fairly sure this isn't used
	"babel-plugin-jest-hoist": "^23.2.0",

I then copied across the react-native/jest/preprocessor.js, and updated it to add [require('babel-plugin-jest-hoist')], at the end of the plugins list.

Running yarn jest now results in TypeError: Cannot assign to read only property 'undefined' of object '#<Object>' from all test suites.

image

I've absolutely no idea what's going on here. My metro version is metro@0.38.4 if that makes any difference.

@giancarlo88
Copy link

I was also having issues with the internal preprocessor (after applying the fix suggested in #19859 (comment)), because of the missing babel-plugin-jest-hoist.

Doing what @deecewan suggested actually did work for me and fixed the tests that had broken. So if you're struggling like I was it may be worth a try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't run jest tests with 0.56.0
10 participants