Skip to content
This repository has been archived by the owner on Sep 11, 2018. It is now read-only.

Hot/Incremental testing is currently broken #871

Closed
drichard opened this issue Jun 20, 2016 · 7 comments
Closed

Hot/Incremental testing is currently broken #871

drichard opened this issue Jun 20, 2016 · 7 comments

Comments

@drichard
Copy link

This is not a major issue, but I just wanted to share my findings since I spent some time today debugging why hot testing is not working anymore.

How to reproduce:

  1. Check out the source
  2. Run npm run test:dev
  3. Make a change to a .spec file
  4. Expect that only the tests for the .spec file are executed

This is a regression as this feature used to work before. Here's what happened:

This starter kit ships with https://github.com/aaronjensen/karma-webpack which enables hot testing. #572 had a problem where the build was broken and it was fixed by this commit e5ca664.

This commit replaces const __karmaWebpackManifest__ = [] with const __karmaWebpackManifest__ = new Array() in test-bundler.js. This makes the tests compile again but at the same breaks hot testing because of the way karma-webpack works. karma-webpack actually parses the file and string replaces __karmaWebpackManifest__ = [] with the list of files to test (see https://github.com/aaronjensen/karma-webpack/blob/master/index.js#L241).

So in order to get hot testing to work we need to revert the change but need to figure out why it breaks the build. I can actually reproduce that npm test fails with __karmaWebpackManifest__ = [] on my machine (OSX, node 5.10.1, npm 3.8.3).

20 06 2016 17:10:39.173:INFO [PhantomJS 2.1.1 (Mac OS X 0.0.0)]: Connected on socket /#W0RkVPTegbppEO_0AAAA with id 81782802
PhantomJS 2.1.1 (Mac OS X 0.0.0) ERROR
  SyntaxError: Expected token ']'
  at /Users/drichard/src/react-redux-starter-kit/tests/test-bundler.js:55 <- webpack:///tests/test-bundler.js:6:0

What's interesting is that npm run test:dev works without any issues. What does test:dev do differently? It doesn't calculate code coverage.

Sure enough if we disable coverage for npm test (for example by removing the isparta loader in karma.conf.js) the tests run just fine.

I'm not entirely sure what's happening here but it seems that both karma-webpack and isparta modify the source file in a way that breaks it (have a look at the compiled output: https://gist.github.com/drichard/14362488a37f67a48b5569b1eee5994f).

In short:

  1. code coverage + aaron jensen's karma-webpack fork can break the build
  2. The workaround disabled hot testing
  3. If you want hot testing, remove the work around and and turn off code coverage (until we figure out why it's broken in the first place)
@dvdzkwsk
Copy link
Owner

Wow, this is superb, thank you so much for the writeup. I never investigated it this thoroughly, so this is news to me. Will fix this today unless you'd like to submit a PR for it.

Again, super, super appreciated.

@drichard
Copy link
Author

Thanks @davezuko! This kit has already saved us a ton of hours by providing an excellent foundation, happy to help.

Not sure yet how to best fix it to be honest. Please go ahead if you have any ideas. :)

@gerbal
Copy link

gerbal commented Jul 9, 2016

I've dug into this a little bit.

Incremental testing works again if the line const __karmaWebpackManifest__ = []; is terminated with a semicolon. But not if the line is const __karmaWebpackManifest__ = new Array().

This keeps #572 fixed and reenables hot testing.

@gerbal
Copy link

gerbal commented Jul 14, 2016

Aand I was wrong about how to fix this.

It looks like some part of the Coverage tests cause the parser to fail (in some environments) when parsing test-bundler.js.

@gvaldambrini
Copy link

On my OSX (node 6.3.1, npm 3.10.6) I had the same parsing error mentioned above running npm run test. The problem disappears replacing isparta with babel-plugin-istanbul as already suggested by a warning printed on another machine (still OSX, still node 6) that does not trigger the error. If interested I can submit a pull request with the mentioned changes.

@dvdzkwsk
Copy link
Owner

isparta has been replaced with babel-plugin-istanbul in this project.

@SpencerCDixon
Copy link
Contributor

is there a PR that shows how to implement istanbul? Getting the same isparta errors :(

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