WIP: trying to upgrade AVA #5

kentcdodds · 2016-03-17T20:00:59Z

Some output before these changes:

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

> [email protected] cover /Users/kdodds/Developer/react-ava-workshop
> nyc --reporter=lcov --reporter=text --reporter=html npm run test


> [email protected] 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

> [email protected] 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

> [email protected] cover /Users/kdodds/Developer/react-ava-workshop
> nyc --reporter=lcov --reporter=text --reporter=html npm run test


> [email protected] 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! [email protected] cover: `nyc --reporter=lcov --reporter=text --reporter=html npm run test`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] 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 · 2016-03-17T20:05:11Z

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 😄

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 · 2016-03-17T20:06:16Z

Another person who may have thoughts: @xjamundx

kentcdodds · 2016-03-17T20:32:47Z

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:

We're on the latest versions of both ava and nyc
$ npm t works 👍
$ npm run cover does not work :-(
The Customers.test.js tests report coverage just fine
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 · 2016-03-17T20:40:40Z

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 · 2016-03-18T04:53:28Z

@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?

@novemberborn

This fixes an `nyc` compatibility issue that [surfaced here](kentcdodds/react-ava-workshop#5) I am not entirely sure why source-map inlining is required (it worked fine without it in previous versions of AVA). @novemberborn - It looks like the type of value we return from `retrieveSourceMap` inside `test-worker.js` changed from an object to a string sometime between `0.9.2` and `0.13` - I wonder if that has something to do with it.

jamestalmage · 2016-03-18T06:16:03Z

@kentcdodds, got a hotfix for you:

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

@novemberborn

See my commit message in jamestalmage/ava@919a131

kentcdodds · 2016-03-18T06:55:55Z

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

kentcdodds · 2016-03-18T06:56:03Z

Thanks for looking into this by the way! 🎉

jamestalmage · 2016-03-18T12:32:27Z

Clear node_modules/.cache

kentcdodds · 2016-03-18T14:30:37Z

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

jamestalmage · 2016-03-18T16:44:20Z

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 · 2016-03-18T17:50:19Z

@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 · 2016-03-18T17:52:38Z

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

novemberborn · 2016-03-20T19:19:03Z

@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 · 2016-03-20T19:21:33Z

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 · 2016-03-20T21:26:14Z

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 · 2016-03-20T21:28:16Z

If course :-)

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

novemberborn · 2016-03-20T21:30:46Z

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

Please do!

kentcdodds · 2016-03-20T21:31:27Z

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

novemberborn · 2016-03-20T21:32:34Z

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 · 2016-03-20T21:48:21Z

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 · 2016-03-21T17:22:35Z

See avajs/ava#662 for an AVA fix that allows nyc to access the source maps.

jamestalmage mentioned this pull request Mar 18, 2016

hotfix: inline source maps avajs/ava#655

Closed

novemberborn mentioned this pull request Mar 21, 2016

Ensure source map can be resolved from cached code avajs/ava#662

Merged

kentcdodds mentioned this pull request Apr 4, 2016

add more exclude glob patterns istanbuljs/nyc#199

Merged

update to latest versions of dependencies

21c1a10

kentcdodds force-pushed the pr/update-ava branch from 4965aa8 to 21c1a10 Compare April 4, 2016 22:26

kentcdodds merged commit 21c1a10 into master Apr 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: trying to upgrade AVA #5

WIP: trying to upgrade AVA #5

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

bcoe commented Mar 18, 2016

jamestalmage commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

jamestalmage commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

jamestalmage commented Mar 18, 2016

novemberborn commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

novemberborn commented Mar 20, 2016

kentcdodds commented Mar 20, 2016

novemberborn commented Mar 20, 2016

kentcdodds commented Mar 20, 2016

novemberborn commented Mar 20, 2016

kentcdodds commented Mar 20, 2016

novemberborn commented Mar 20, 2016

novemberborn commented Mar 20, 2016

novemberborn commented Mar 21, 2016

WIP: trying to upgrade AVA #5

WIP: trying to upgrade AVA #5

Conversation

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

kentcdodds commented Mar 17, 2016

bcoe commented Mar 18, 2016

jamestalmage commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

jamestalmage commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

jamestalmage commented Mar 18, 2016

novemberborn commented Mar 18, 2016

kentcdodds commented Mar 18, 2016

novemberborn commented Mar 20, 2016

kentcdodds commented Mar 20, 2016

novemberborn commented Mar 20, 2016

kentcdodds commented Mar 20, 2016

novemberborn commented Mar 20, 2016

kentcdodds commented Mar 20, 2016

novemberborn commented Mar 20, 2016

novemberborn commented Mar 20, 2016

novemberborn commented Mar 21, 2016