search

kentcdodds / react-ava-workshop

:tiger: A workshop repository for testing React ⚛ with AVA :rocket: --> slides
http://kcd.im/react-ava
MIT License
190 stars 19 forks source link

WIP: trying to upgrade AVA #5

Closed kentcdodds closed 8 years ago

kentcdodds commented 8 years ago

Some output before these changes:

~/Developer/react-ava-workshop (master)
🔥  $ npm run cover

> react-ava-workshop@1.0.0 cover /Users/kdodds/Developer/react-ava-workshop
> nyc --reporter=lcov --reporter=text --reporter=html npm run test

> react-ava-workshop@1.0.0 test /Users/kdodds/Developer/react-ava-workshop
> ava 'app/**/*.test.js' --verbose --require ./other/setup-ava-tests.js

  ✔ store › Customers › customers should start with empty
  ✔ store › Customers › setting customers and getting them
  ✔ store › Customers › subscribing to the store
Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `ListOfCustomers`. See https://fb.me/react-warning-keys for more information.
  ✔ components › Toggle › toggle--off class applied by default
  ✔ components › Toggle › toggle--on class applied when initialToggledOn specified to true
  ✔ components › Toggle › invokes the onToggle prop when clicked
  ✔ containers › CustomerList › Renders no customers and add button
  ✔ containers › CustomerList › Renders customers and add button
  ✔ containers › CustomerList › Responds to store updates
  ✔ containers › CustomerList › unsubscribes when unmounted

  10 tests passed

------------------|----------|----------|----------|----------|----------------|
File              |  % Stmts | % Branch |  % Funcs |  % Lines |Uncovered Lines |
------------------|----------|----------|----------|----------|----------------|
 components/      |      100 |      100 |      100 |      100 |                |
  Toggle.js       |      100 |      100 |      100 |      100 |                |
 containers/      |      100 |      100 |      100 |      100 |                |
  CustomerList.js |      100 |      100 |      100 |      100 |                |
 store/           |      100 |      100 |      100 |      100 |                |
  Customers.js    |      100 |      100 |      100 |      100 |                |
------------------|----------|----------|----------|----------|----------------|
All files         |      100 |      100 |      100 |      100 |                |
------------------|----------|----------|----------|----------|----------------|

Some output after these changes:

~/Developer/react-ava-workshop (pr/update-ava)
🔥  $ npm run test

> react-ava-workshop@1.0.0 test /Users/kdodds/Developer/react-ava-workshop
> ava 'app/**/*.test.js' --verbose --require ./other/setup-ava-tests.js

  ✔ store › Customers › customers should start with empty
  ✔ store › Customers › setting customers and getting them
  ✔ store › Customers › subscribing to the store
  ✔ components › Toggle › toggle--off class applied by default
  ✔ containers › CustomerList › Renders no customers and add button
  ✔ components › Toggle › toggle--on class applied when initialToggledOn specified to true
Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `ListOfCustomers`. See https://fb.me/react-warning-keys for more information.
  ✔ containers › CustomerList › Renders customers and add button
  ✔ components › Toggle › invokes the onToggle prop when clicked
  ✔ containers › CustomerList › Responds to store updates
  ✔ containers › CustomerList › unsubscribes when unmounted

  10 tests passed

~/Developer/react-ava-workshop (pr/update-ava)
🔥  $ npm run cover

> react-ava-workshop@1.0.0 cover /Users/kdodds/Developer/react-ava-workshop
> nyc --reporter=lcov --reporter=text --reporter=html npm run test

> react-ava-workshop@1.0.0 test /Users/kdodds/Developer/react-ava-workshop
> ava 'app/**/*.test.js' --verbose --require ./other/setup-ava-tests.js

  ✔ store › Customers › customers should start with empty
  ✔ store › Customers › setting customers and getting them
  ✔ store › Customers › subscribing to the store
  ✔ containers › CustomerList › Renders no customers and add button
  ✔ components › Toggle › toggle--off class applied by default
  ✔ components › Toggle › toggle--on class applied when initialToggledOn specified to true
Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `ListOfCustomers`. See https://fb.me/react-warning-keys for more information.
  ✔ containers › CustomerList › Renders customers and add button
  ✔ containers › CustomerList › Responds to store updates
  ✔ components › Toggle › invokes the onToggle prop when clicked
  ✔ containers › CustomerList › unsubscribes when unmounted

  10 tests passed

/Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:241
            text = structuredText[startLine].text;
                                            ^

TypeError: Cannot read property 'text' of undefined
    at /Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:241:45
    at Array.forEach (native)
    at annotateFunctions (/Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:224:26)
    at HtmlReport.Report.mix.writeDetailPage (/Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:427:9)
    at /Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:489:26
    at SyncFileWriter.extend.writeFile (/Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/util/file-writer.js:57:9)
    at FileWriter.extend.writeFile (/Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/util/file-writer.js:147:23)
    at /Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:488:24
    at Array.forEach (native)
    at HtmlReport.Report.mix.writeFiles (/Users/kdodds/Developer/react-ava-workshop/node_modules/istanbul/lib/report/html.js:482:23)

npm ERR! Darwin 14.5.0
npm ERR! argv "/Users/kdodds/.nvm/versions/node/v4.3.2/bin/node" "/Users/kdodds/.nvm/versions/node/v4.3.2/bin/npm" "run" "cover"
npm ERR! node v4.3.2
npm ERR! npm  v3.8.0
npm ERR! code ELIFECYCLE
npm ERR! react-ava-workshop@1.0.0 cover: `nyc --reporter=lcov --reporter=text --reporter=html npm run test`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the react-ava-workshop@1.0.0 cover script 'nyc --reporter=lcov --reporter=text --reporter=html npm run test'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the react-ava-workshop package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     nyc --reporter=lcov --reporter=text --reporter=html npm run test
npm ERR! You can get information on how to open an issue for this project with:
npm ERR!     npm bugs react-ava-workshop
npm ERR! Or if that isn't available, you can get their info via:
npm ERR!     npm owner ls react-ava-workshop
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/kdodds/Developer/react-ava-workshop/npm-debug.log

As you can see, the tests work in both cases (yay), but coverage is busted after upgrading to AVA 0.13.0 and I have no idea why. Been working on this for a while now and I'm really stumped.

Thoughts appreciated from my AVA friends :-)

kentcdodds commented 8 years ago

cc @jamestalmage @sindresorhus @sotojuan @novemberborn @vdemedes, if ya'll would have a second to give this a look I'd really appreciate it :-) I'm seeing this in my work project as well.

I've been looking into this for a day now. I don't really have any leads.

Also, maybe @bcoe of the nyc project would have some insights here :smile:

Thanks for any help/tips you can give. This is an instructional repository and I'd love to be able to remove this section about having to install 9.2.0 when teaching people how to use AVA with React :-)

kentcdodds commented 8 years ago

Another person who may have thoughts: @xjamundx

kentcdodds commented 8 years ago

I think that this is an inter-op issue with React + AVA + nyc somehow...

Here are a few commands you can run to get a quick-start on this:

git clone -b pr/update-ava https://github.com/kentcdodds/react-ava-workshop.git
cd react-ava-workshop/
npm i
./node_modules/.bin/nyc --version
./node_modules/.bin/ava --version
npm t
npm run cover
./node_modules/.bin/nyc --reporter=lcov ./node_modules/.bin/ava app/**/Customers.test.js --require ./other/setup-ava-tests.js
./node_modules/.bin/nyc --reporter=lcov ./node_modules/.bin/ava app/**/CustomerList.test.js --require ./other/setup-ava-tests.js

Here's the output I see with those last two commands:

~/Desktop/react-ava-workshop (pr/update-ava)
🍰  $ ./node_modules/.bin/nyc --reporter=lcov ./node_modules/.bin/ava app/**/Customers.test.js --require ./other/setup-ava-tests.js

   3 passed

~/Desktop/react-ava-workshop (pr/update-ava)
🍰  $ ./node_modules/.bin/nyc --reporter=lcov ./node_modules/.bin/ava app/**/CustomerList.test.js --require ./other/setup-ava-tests.js
Warning: Each child in an array or iterator should have a unique "key" prop. Check the render method of `ListOfCustomers`. See https://fb.me/react-warning-keys for more information.

   4 passed

/Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:241
            text = structuredText[startLine].text;
                                            ^

TypeError: Cannot read property 'text' of undefined
    at /Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:241:45
    at Array.forEach (native)
    at annotateFunctions (/Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:224:26)
    at HtmlReport.Report.mix.writeDetailPage (/Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:427:9)
    at /Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:489:26
    at SyncFileWriter.extend.writeFile (/Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/util/file-writer.js:57:9)
    at FileWriter.extend.writeFile (/Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/util/file-writer.js:147:23)
    at /Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:488:24
    at Array.forEach (native)
    at HtmlReport.Report.mix.writeFiles (/Users/kdodds/Desktop/react-ava-workshop/node_modules/istanbul/lib/report/html.js:482:23)

A few things you'll notice:

  1. We're on the latest versions of both ava and nyc
  2. $ npm t works :+1:
  3. $ npm run cover does not work :-(
  4. The Customers.test.js tests report coverage just fine
  5. The CustomerList.test.js tests fail when reporting coverage

The most noticeable difference between those two files is one has JSX and the other does not. I'm thinking that may have something to do with it...

kentcdodds commented 8 years ago

Note: The AVA docs say that for code coverage you may need to specify inline source maps for coverage to work. But adding this to my .babelrc doesn't seem make a difference:

"sourceMaps": "inline"
bcoe commented 8 years ago

@novemberborn any thoughts on this one, looks like startLine was invalid? I thought we put some logic in to pin to a valid line number a while back?

jamestalmage commented 8 years ago

@kentcdodds, got a hotfix for you:

npm i -D jamestalmage/ava#hotfix/inline-sourcemaps

@novemberborn

See my commit message in https://github.com/jamestalmage/ava/commit/919a1318cbca11fe9bc8f1da5f2ee5c77d80a4bb

kentcdodds commented 8 years ago

@jamestalmage, I updated the PR and it's still giving me the same error. Am I doing something wrong?

kentcdodds commented 8 years ago

Thanks for looking into this by the way! :tada:

jamestalmage commented 8 years ago

Clear node_modules/.cache

kentcdodds commented 8 years ago

That worked! Any chance this will get patch released today? Or should I just go with the hotfix for now?

jamestalmage commented 8 years ago

I'm not entirely sure my solution is the correct one (I think the problem is elsewhere), so hotfix.

I've opened an AVA PR to discuss

novemberborn commented 8 years ago

@kentcdodds I have some ideas on what's going here but haven't yet had a chance to dig deeper. I can reproduce the issue locally though so that's good.

kentcdodds commented 8 years ago

Thanks for taking the time to look at it :-)

novemberborn commented 8 years ago

@kentcdodds nyc is covering your test files and then fails to apply the source maps. That's a separate issue I haven't tracked down yet.

To exclude your test files add the following to your package.json:

  "nyc": {
    "exclude": "app/**/*.test.js"
  }

Normally nyc excludes ['test', 'test{,-*}.js'] but your tests are in an unusual spot (to me at least).

kentcdodds commented 8 years ago

Oh interesting. Didn't even realize that I was instrumenting the test files. I'll give that a look. But this didn't actually solve the root issue correct? You're still looking at that?

novemberborn commented 8 years ago

But this didn't actually solve the root issue correct? You're still looking at that?

Indeed. Regardless of the root issue though you probably don't want to be instrumenting the test files.

kentcdodds commented 8 years ago

If course :-)

Mind if I submit a pull request to support my naming convention for test files?

novemberborn commented 8 years ago

Mind if I submit a pull request to support my naming convention for test files?

Please do!

kentcdodds commented 8 years ago

I'm sure I could find it, but if it's quick for you, I'd appreciate a link to the relevant code :-)

novemberborn commented 8 years ago

I'm sure I could find it, but if it's quick for you, I'd appreciate a link to the relevant code :-)

https://github.com/bcoe/nyc/blob/69ed03b29c423c0fd7bd41f9dc8e7a3a68f7fe50/index.js#L45

I'll leave the test location for you ;-)

novemberborn commented 8 years ago

Looks like nyc cannot find the source map AVA generated for the test file. Unfortunately nyc thinks it instrumented the original test file but it didn't. Without the source map it can't generate a coverage report because the information generated by Istanbul does not line with the test file.

This might need some changes both to AVA and nyc. Good thing I have commit access to both ;-)

novemberborn commented 8 years ago

See https://github.com/sindresorhus/ava/pull/662 for an AVA fix that allows nyc to access the source maps.