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.

Sign up for GitHub

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

Jump to bottom

Regroup #175

Closed
bclinkinbeard opened this issue Sep 23, 2017 · 16 comments
Closed

Regroup #175

bclinkinbeard opened this issue Sep 23, 2017 · 16 comments

Comments

@bclinkinbeard
Copy link
Contributor

Things have gotten into a weird state and I think we need to resolve some issues before going any further. I don't know where the issues are coming from but I flailed all day yesterday with bizarre issues and failures. Between Lerna, Babel, Jest, and React, I don't know where the issues lie but they're disconcerting and super frustrating. I think we need to get all the tests working before we do anything else.

  • The idyll-components and idyll-document tests are failing, even after fix tests with babel + jest #174
  • I have no idea why these tests are failing, and they are generating errors that don't make any sense. Things like TypeError: Path must be a string. Received undefined, Cannot find module 'fbjs/lib/warning' from 'ReactNoopUpdateQueue.js', etc.

Some notes:

  • Jest will automatically set NODE_ENV to test so we shouldn't need to call anything but jest from the package.json.
  • fix tests with babel + jest #174 added babel-jest but that is supposed to come with jest by default
  • fix tests with babel + jest #174 tries to use babel-preset-es2015 when running the tests but it's not listed as a dependency and it's deprecated
@mathisonian
Copy link
Member

I made those changes in 174 mostly based off of jestjs/jest#3202 and jestjs/jest#770, and from this section of the README: https://github.com/facebook/jest#using-babel there is one alternate approach listed there which is

// package.json
{
  "jest": {
    "transform": {}
  }
}

Can you track down any specific lines of code with the error messages? Are those Idyll errors, or coming from the test runner? I can't really tell if these are still babel/jest errors you are seeing or something else.

FWIW - I just tried cloning from master, running yarn install and then running the tests and things seem to be working as expected. The notes about babel-preset-es2015, is a little weird though... maybe we can switch to the config i posted above

@bclinkinbeard
Copy link
Contributor Author

The idyll-components and idyll-document tests are passing for you? I check out master, install deps, and get this. No other info.

~/Code/idyll/packages/idyll-components master*
❯ yarn test
yarn test v1.0.2
$ cross-env BABEL_ENV=test jest
 FAIL  test/components.js
  ● Test suite failed to run

    TypeError: Path must be a string. Received undefined

      at assertPath (path.js:28:11)
      at Object.relative (path.js:1264:5)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        0.646s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

And

~/Code/idyll/packages/idyll-document master*
❯ yarn test
yarn test v1.0.2
$ cross-env BABEL_ENV=test jest
 FAIL  test/component.js
  ● Test suite failed to run

    Cannot find module 'fbjs/lib/warning' from 'ReactNoopUpdateQueue.js'

      at Resolver.resolveModule (../../node_modules/jest-runtime/node_modules/jest-resolve/build/index.js:192:17)
      at Object.<anonymous> (../../node_modules/react/lib/ReactNoopUpdateQueue.js:13:15)

 FAIL  test/vars.js
  ● Test suite failed to run

    Cannot find module 'fbjs/lib/warning' from 'ReactNoopUpdateQueue.js'

      at Resolver.resolveModule (../../node_modules/jest-runtime/node_modules/jest-resolve/build/index.js:192:17)
      at Object.<anonymous> (../../node_modules/react/lib/ReactNoopUpdateQueue.js:13:15)

 FAIL  test/nodes.js
  ● Test suite failed to run

    Cannot find module 'fbjs/lib/warning' from 'ReactNoopUpdateQueue.js'

      at Resolver.resolveModule (../../node_modules/jest-runtime/node_modules/jest-resolve/build/index.js:192:17)
      at Object.<anonymous> (../../node_modules/react/lib/ReactNoopUpdateQueue.js:13:15)

Test Suites: 3 failed, 3 total
Tests:       0 total
Snapshots:   0 total
Time:        0.931s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

@mathisonian
Copy link
Member

mathisonian commented Sep 23, 2017 via email

@bclinkinbeard
Copy link
Contributor Author

bclinkinbeard commented Sep 23, 2017 via email

@mathisonian
Copy link
Member

No, I'm not using any lerna commands or anything complicated - just

  • yarn install
  • cd packages/<package-name>
  • npm test

I'm on node v6.9.4 and yarn 0.27.5 with workspaces enabled. These are a little old, but these tests passing makes me thing its not a node or yarn version thing (that tests node 6-8, using yarn 1.0.1.

These errors aren't exactly what your seeing, but seem close jestjs/jest#2218

Do you see the same errors with a completely clean clone from github?

@mathisonian
Copy link
Member

Also, besides the tests are things working for you? Can you run the editor?

To me this seems like either an npm/dependency error, or something with jest+babel that for some reason is working for me but still has issues

@bclinkinbeard
Copy link
Contributor Author

Nope, running the editor says undefined. (While processing preset: "/Users/bclinkinbeard/Code/idyll/packages/idyll-document/.babelrc.js") Unsupported preset format

@mathisonian
Copy link
Member

Okay - I'm seeing the same thing there at least. I think because BABEL_ENV isn't set and we end up with an undefined here. I'll push a fix for that, and see if I can't get jest working using this package.json method

// package.json
{
  "jest": {
    "transform": {}
  }
}

instead of the babelrc method

@bclinkinbeard
Copy link
Contributor Author

A fresh clone doesn't work either, but I also can't run react-router's tests so it feels like my system is just hosed.

I don't really understand the whole empty transform thing, but it might be worth simplifying the .babelrc to match what they show at https://facebook.github.io/jest/docs/en/getting-started.html#using-babel.

{
  "presets": [["es2015", { "modules": false }], "react"],
  "env": {
    "test": {
      "presets": [["es2015"], "react"]
    }
  }
}

Taking .babelrc.js out of the picture entirely. I also had some mixed success with jest 20.0.4 so it might be worth trying to roll that back? I'm grasping at straws at this point.

@mathisonian
Copy link
Member

Adding a fallback default to .babelrc.js gets the editor working okay, (can't seem to get the package.json version to work)

const BABEL_ENV = process.env.BABEL_ENV;
const env = {
  cjs: 'env',
  es: [
    'env', { loose: true, modules: false }
  ],
  test: [
    'es2015'
  ]
}[BABEL_ENV || 'cjs'];

module.exports = {
  plugins: ['transform-object-rest-spread'],
  presets: [
    env,
    'react',
  ],
};

using that simplified one you posted above i'm getting errors for missing object spread

  51 |     // left and right props assume no horizontal scrolling
  52 |     refs[domNode.dataset.ref] = {
> 53 |       ...bools,
     |       ^
  54 |       domNode,
  55 |       portionInView,
  56 |       size: {
[0004] 619ms      7523B GET    200 /index.js (generated)

one thing that I was doing when testing yesterday was just hardcoding the babel config (taking the BABEL_ENV option out completely to make sure there weren't issues coming from that variable).

I'm pretty confused at this point

@bclinkinbeard
Copy link
Contributor Author

How does this make ANY sense at all??

image

@mathisonian
Copy link
Member

😕

We have an older jest dependency in idyll-cli, my only possible guess is that yarn hoisted v21 to the workspace level and that older v19 one was then installed locally due to idyll-cli being installed?? that doesn't really make sense but i can't think of any other explanation

@bclinkinbeard
Copy link
Contributor Author

Isolated repo showing the/similar issue. https://github.com/bclinkinbeard/wth

@bclinkinbeard
Copy link
Contributor Author

96e5f78 is the last commit where I can get the tests to pass.

@mathisonian
Copy link
Member

What should I be seeing in that repo? I'm getting

screen shot 2017-09-25 at 8 39 02 am

and then after running yarn add react-test-renderer the tests pass

screen shot 2017-09-25 at 8 39 50 am

@bclinkinbeard
Copy link
Contributor Author

OK, things seem to be stable now, thank glob. For posterity, Jest v21.1.0 seemed to be the culprit as rolling back to v20.0.4 (and removing babel-jest, which was added as an attempted fix) got things running smooth again. #177 is where the magic happened.

I plan to expand the tests significantly in the coming days, and we shall never again merge a PR that is failing CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@bclinkinbeard @mathisonian