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

Tests are failing after upgrade to 0.8.1 #1156

Closed
just-boris opened this issue Dec 5, 2016 · 44 comments · Fixed by #1179
Closed

Tests are failing after upgrade to 0.8.1 #1156

just-boris opened this issue Dec 5, 2016 · 44 comments · Fixed by #1179
Milestone

Comments

@just-boris
Copy link
Contributor

just-boris commented Dec 5, 2016

If you are reporting a bug, please fill in below. Otherwise feel free to remove this template entirely.

Can you reproduce the problem with latest npm?

I have Yarn installed, so CRA used latest yarn@0.18.0 to setup.

Description

My test command fails with the following error

 FAIL  src/App.test.js
  ● Test suite failed to run

    TypeError: /home/bserdiuk/coding/test/src/App.test.js: Cannot read property 'length' of null
      
      at Buffer._append (node_modules/babel-generator/lib/buffer.js:83:25)
      at Buffer.append (node_modules/babel-generator/lib/buffer.js:55:10)
      at Generator._append (node_modules/babel-generator/lib/printer.js:206:52)
      at Generator.word (node_modules/babel-generator/lib/printer.js:131:10)
      at Generator.Identifier (node_modules/babel-generator/lib/generators/types.js:38:8)
      at node_modules/babel-generator/lib/printer.js:298:23
      at Buffer.withSource (node_modules/babel-generator/lib/buffer.js:142:28)
      at Generator.withSource (node_modules/babel-generator/lib/printer.js:189:15)
      at Generator.print (node_modules/babel-generator/lib/printer.js:297:10)
      at Generator.VariableDeclarator (node_modules/babel-generator/lib/generators/statements.js:309:8)

It worked fine with version 0.7.0, today I tried to upgrade to latest 0.8.1 and got the error above.
I have generated new test application with latest create-react-app and react-scripts and could find a minimal example to reproduce.

Add the following test in your test files, then run npm test.

it('async test', async () => {
    const {result} = doSometing();
});

Basically, destructuring assignment doesn't work in async function with Jest.
Babel Repl transpiles this code fine. Also it works fine if I will put this code into source. Problem appears only in testing code.

Expected behavior

I expect that it works as it was before in 0.7.0

Actual behavior

Tests are not run, error appears before any test starts.

Environment

Run these commands in the project folder and fill in their results:

  1. npm ls react-scripts (if you haven’t ejected):
npm ls react-scripts
test@0.1.0 /home/bserdiuk/coding/test
└── react-scripts@0.8.1 
  1. node -v:
v6.9.1
  1. npm -v:
4.0.3

Then, specify:

  1. Operating system: Ubuntu 16.04.1 LTS
  2. Browser and version: not applicable
@gaearon gaearon modified the milestones: 0.8.1, 0.8.2 Dec 5, 2016
@EnoahNetzach
Copy link
Contributor

probably related: #1160

@just-boris
Copy link
Contributor Author

just-boris commented Dec 6, 2016

I was following the discussion in the thread #1160 and believe that my problem is also related to mentioned Babel issue: babel/babel#4759

Also, I have found that it was introduced by updating preset:
#1051

Now there are less Babel plugins applied in test environment, but some of them can't still have implicit dependencies on other transformations that was excluded now.

@gaearon could you consider rolling this PR back to work around the issue?

@EnoahNetzach
Copy link
Contributor

@just-boris yep, that's exactly what we were finding out in #1160, also.

So, yep, my advise is that we either:

  • rollback to using babel-preset-latest instead of babel-preset-env
  • manually add the "missing" plugins

I'd prefer the first solution.

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

I'm happy to push a fix but I want to have an actionable plan for re-enabling -env preset so that we can keep track of all issues and know when it's safe to re-enable. Kinda like we have a list in #553 (comment).

manually add the "missing" plugins

I would prefer this to enabling all of them. There are speed improvements from disabling most transforms, and I don't want to lose them. If something is missing we'll learn about it and file more Babel bugs. It's better than not knowing and learning that the next time we try to enable -env.

Would you like to make a PR for adding missing plugins?

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

Also, would that help as a temporary workaround?
babel/babel#4759 (comment)

@EnoahNetzach
Copy link
Contributor

It doesn't seem to work in my case..

@hzoo
Copy link

hzoo commented Dec 6, 2016

Just saw this - so tried to repro from scratch

npm install -g create-react-app
create-react-app my-app
cd my-app/
npm start

I added a test to App.test.js

it('async test', async () => {
    const {result} = Promise.resolve({ result: 1});
});

and it passes for me with yarn run test on yarn 0.17.4 and node 6.9.0

but maybe it was just that code?

@hzoo
Copy link

hzoo commented Dec 6, 2016

env means that you shouldn't need regenerator at all because you are using -env? (which just uses async-to-gen)

@EnoahNetzach
Copy link
Contributor

@hzoo the problem seems to arise when babel-jest tries to transpile code, not while running the proper tests (I'm not entirely sure, as I don't know exactly how it works).

Try this.

@hzoo
Copy link

hzoo commented Dec 6, 2016

Ok that errors - it's a different error than the OP's error.

And i'm pretty sure it's just because regenerator is being run from another plugin somehow because if you are using node 6.9.0 it wouldn't be running it via babel-preset-env which is the point

@EnoahNetzach
Copy link
Contributor

EnoahNetzach commented Dec 6, 2016

@hzoo I already tried to disable transform-regenerator, leaving only env and, while I don't have astync & destructuring problems anymore, I get

TypeError: _definition2.default is not a constructor

and

var addIndex = function addIndex({ index, ...routes } = {}, base)
                                          ^^^
    SyntaxError: Unexpected token ...

errors.

@hzoo
Copy link

hzoo commented Dec 6, 2016

https://github.com/facebookincubator/create-react-app/blob/11cd813802153a5de0ec1d60ea764fd1b26be8c8/packages/babel-preset-react-app/index.js#L26-L30, it's included in plugins all the time so we wouldn't want this if we are also using env.

Otherwise it wouldn't compile async since it says async: false

@hzoo
Copy link

hzoo commented Dec 6, 2016

Basically what I think we need to do is make sure the plugins: plugins don't have any plugins that babel-preset-env already covers (which is everything that is spec/latest).

So at the very least remove transform-regenerator in test since it would be included or removed via env

@just-boris
Copy link
Contributor Author

@hzoo

Just saw this - so tried to repro from scratch
I added a test to App.test.js
and it passes for me with yarn run test on yarn 0.17.4 and node 6.9.0

I followed the same steps, (except that I have node 6.9.1) and got the error. Does it help you if will send the result npm ls | grep babel for you?

@hzoo
Copy link

hzoo commented Dec 6, 2016

@EnoahNetzach
Copy link
Contributor

@hzoo so I should be able to solve this by removing L26-L30?

If so, I tried it and it doesn't work 😢

@hzoo
Copy link

hzoo commented Dec 6, 2016

I cloned the repo you linked and commented it out and it worked. Maybe rerun yarn run test and also clear node_modules/.cache/

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

I can confirm that commenting out regenerator fixes

it('async test', async () => {
    const {result} = await doSometing(); // Note: MUST have await to reproduce
});

for me.

Before commenting it I was seeing

  /Users/gaearon/p/create-react-app/packages/react-scripts/template/src/App.test.js: Expected type "Expression" with option {}

but not anymore.

One thing to keep in mind is there's some Jest caching going on so you need to change something in test file every time you try after changing preset.

@EnoahNetzach
Copy link
Contributor

Yes, it fixes that error, but not the other ones

@hzoo
Copy link

hzoo commented Dec 6, 2016

Ok then that's a different issue and unrelated to what was reported above? I didn't know when you said it doesn't work what you meant

var addIndex = function addIndex({ index, ...routes } = {}, base)
                                          ^^^
    SyntaxError: Unexpected token ...

I don't see that in the repo you posted?

@EnoahNetzach
Copy link
Contributor

you are right, I'm trying to get on that repo all the errors I'm finding in my codebase, I'll update it asap

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

This file compiles for me after removing regenerator on Node 6.9.1:

import React from 'react';
import ReactDOM from 'react-dom';
import App from './App';

function doSometing() {
  return {};
}

const getId = async () => {
  const { id } = await dispatch(42)
  return id
};

const getId2 = async () => {
  const p = await dispatch(42)
  const { id } = p
  return id
};

function Record() {
  return function() {}
}

const User = new Record({ id: NaN })
new User({ id: 42 })

it('async test', async () => {
    const {resultxx} = await doSometing();
});

What are other cases? That’s all I could find from this issue and #1160.

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

I can confirm

var addIndex = function addIndex({ index, ...routes } = {}, base) {
};

is not working.

    /Users/gaearon/p/create-react-app/packages/react-scripts/template/src/App.test.js:27
    var addIndex = function addIndex({ index, ...routes } = {}, base) {
                                              ^^^
    SyntaxError: Unexpected token ...
      
      at transformAndBuildScript (../node_modules/jest-runtime/build/transform.js:316:10)
      at process._tickCallback (../../../internal/process/next_tick.js:103:7)

@hzoo
Copy link

hzoo commented Dec 6, 2016

Oh right that specifically babel/babel#4851! Haven't had time to fix but can work on that

(edge cases in the object-rest-spread transforms)

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

@EnoahNetzach @just-boris

Can you provide any other cases for future reference? I'll keep parameters and destructuring for babel/babel#4851 but I need to know if there's anything else failing.

All of #1156 (comment) appears solved by excluding regenerator from default transform.

@EnoahNetzach
Copy link
Contributor

@gaearon I'm still having the TypeError: _definition2.default is not a constructor with immutable, but frankly I've not been able to reproduce it yet..

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

@EnoahNetzach Could it be related to anything else in that file? Maybe you need the whole file (with variable bindings) to reproduce some corner case.

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

@EnoahNetzach Any chance you're also seeing Babel/Jest caching? You'd need to modify the test file to make sure your config changes are picking up.

@EnoahNetzach
Copy link
Contributor

EnoahNetzach commented Dec 6, 2016

@gaearon sorry, I confirm everything works for me without regenerator and with parameters (destructuring seems to be useless in this case).

(I was commenting out plugins.push.apply.........)

@EnoahNetzach
Copy link
Contributor

@gaearon should I re-open #1177 or make another PR?

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

@EnoahNetzach I'm on it, will send something soon for you to review

@EnoahNetzach
Copy link
Contributor

@gaearon great ;)

@hzoo
Copy link

hzoo commented Dec 6, 2016

So there are 2 separate issues? one relating to -env and regenerator and the other is using object-rest-spread and there being a bug with the standalone plugin.

You should be able to to just remove regenerator since it's included in both env/latest already and then add destructuring/parameters. Not sure why you need arrow-functions?

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

You should be able to to just remove regenerator since it's included in both env/latest already

If I recall correctly I am explicitly including together with -latest because I wanted to customize its options. I don't remember why very well..

It indeed appears unnecessary with -env. I don't care about options there because transpiled output (and its size or global pollution) doesn't matter to me in tests.

@gaearon
Copy link
Contributor

gaearon commented Dec 6, 2016

@hzoo @EnoahNetzach @just-boris

Ready for review:

#1179

@EnoahNetzach
Copy link
Contributor

@gaearon npm test, npm start and npm run build work for me.

Thanks!

@just-boris
Copy link
Contributor Author

just-boris commented Dec 7, 2016

Works well.
Could you make a release, so I can finally switch to latest version instead of patching source code?

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

I'll make a release in a day or two but I hope to fix a few more bugs. Definitely this week.

@gaearon
Copy link
Contributor

gaearon commented Dec 7, 2016

Fixed in 0.8.2.
Please verify.

https://github.com/facebookincubator/create-react-app/releases/tag/v0.8.2

@pke
Copy link

pke commented Dec 8, 2016

Still getting

import { winner } from "./gameplay";
    ^^^^^^
SyntaxError: Unexpected token import

in VS code jest extension but works on in terminal.

@gaearon
Copy link
Contributor

gaearon commented Dec 8, 2016

Likely some caches. I'm not sure where exactly Jest keeps its cache but try clearing stuff in /tmp/.

@hzoo
Copy link

hzoo commented Dec 8, 2016

@gaearon I opened up jestjs/jest#2231 - to default to ./node_modules/.cache as the directory

@EnoahNetzach
Copy link
Contributor

Right now it should be located in /tmp/jest

@pke
Copy link

pke commented Dec 8, 2016

yes, but clearing cache does not solve the vscode jest extensions problem. Filed a bug over there.

@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants