-
Notifications
You must be signed in to change notification settings - Fork 110
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
[Fixes #111] Update dependencies, get babel instrumenter working #115
[Fixes #111] Update dependencies, get babel instrumenter working #115
Conversation
And latest Babel. There are no tests that currently exercise the Babel instrumenter — that’s next.
Issue 1: Neither ember-cli 2.13 nor yarn support Node 0.12. Is there a reason to keep supporting Node 0.12 for this project? |
@paulcwatts Nope, Node 0.12 can be dropped. |
Is it possible to break this up? It's a little hard to review only the babel changes for me with all the ember-cli update noise. |
@kellyselden I can try that. |
Specifying ‘blacklist’ gave a Babel error because of a deprecation, but removing it doesn’t make the tests fail. Perhaps it isn’t needed in Babel 6?
The trouble is that for apps written against ember-cli < 2.13, we cannot use the app's babel options, including this addon's dummy app. This reveals another issue where simply upgrading this blueprint to 2.13 won't work for apps that are still on older versions ember-cli. We'd need to test against older versions of ember-cli, which is made easier by using ember-try or potentially https://github.com/tomdale/ember-cli-addon-tests. So splitting up this PR is possible, but if the ember-cli upgrade doesn't come first, the babel fix becomes more difficult and involves throw away work. |
@paulcwatts alright lets keep it together then. |
Issue 2: async functions don't seem to be working. |
@paulcwatts Has there been any movement on this since last update? I can attest that as of the last commit in this PR coverage fails for my codebase. I suspect it is async functions as you posited. |
@rondale-sc I haven't had time to work on it since then, I may have time later this week or this weekend. Definitely next week. |
The trouble is that the Babel instrumenter uses esprima for its parser, but esprima doesn't yet support async functions:
jquery/esprima#1597 I can look into switching to using Babylon as the parser, but I don't know yet what that will involve. |
I am able to get them to work by manually adding the async-to-generator plugin to the list of babelOptions when it is invoked by the babel instrumenter here: var plugins = this.babelOptions.plugins;
plugins.push(require('babel-plugin-transform-async-to-generator'));
var result = this._r = (0, babelTransform)(code, extend({}, this.babelOptions, { filename: fileName })); This is ugly, but I don't know why the babel options don't already include a plugin to parse async functions. |
@paulcwatts Have you attempted to upgrade istanbul to alpha? I think maybe this was resolved in their alpha branch: gotwarlost/istanbul#733 Though I could be barking up the wrong tree. |
Does your fix belong in the host application or in this addon? The additional transform linked above I mean. (also, as a side note I believe I spoke too soon on the istanbul alpha upgrade. Seems like that'd only work with node 7+) |
Any updates with this? :D |
This includes the transform to handle async functions when instrumenting.
With the latest push, async functions seem to be working -- at least on one of my projects. I'd appreciate some other folks check to see if this branch works for them! The build failure seems to be a random timeout error: I can't seem to restart the build to see if a rebuild fixes it. |
@paulcwatts Your fix seems to work for async functions, but it doesn't account for other transforms people might be using (decorators, for instance) — maybe users need to be able to just specify a set of Babel plugins in |
@dfreeman Good point. I can add a configuration option. Hopefully it will be temporary, as I'd like to investigate switching to babel-plugin-istanbul. If babel-plugin-instanbul can be used then we can potentially deprecate the new option along with the useBabelInstrumenter option. |
For people who use even newer Javascript features like decorators, this allows the instrumenter to parse the code.
@dfreeman Can you test the latest to see if it works for you? You should just be able to add a |
@paulcwatts It almost works. I tested with It looks like Babel happily accepts just strings for its list of plugins, and then internally handles the |
Some plugins are exported with a “default” key rather than the default. Babel happily accepts just strings for its list of plugins, and then internally handles the default disparity for you. So don’t use require(), just use the plugin name.
@dfreeman I made that change (again, the build failed for intermittent timeout reasons). I temporarily added decorators plugin and ran the tests and it seemed to work. |
@paulcwatts OK. I wasn't aware of that reasoning. The plugin default looks good to me. |
I'll add a comment in the config to that effect. |
Even better! |
1. Remove IDE-specific ignores from project 2. Remove missing welcome-page component from Ember app 3. Add comment around the reasoning for the default plugins list.
@kellyselden Review changes made. |
@rwjblue @kategengler This has been reviewed and approved by @kellyselden, what else needs to happen to get this merged? |
config/ember-try.js
Outdated
@@ -0,0 +1,91 @@ | |||
/* eslint-env node */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be removed, as ember-try isn't used in this repo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point I was thinking that it would be nice to switch to using ember-try for the tests, but until then I'll remove it.
test/helpers/run-command.js
Outdated
@@ -1,5 +1,3 @@ | |||
'use strict'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why all the use strict removals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I first updated the blueprint, there were a bunch of eslint errors around using "use strict" with sourceType: module
. But for some reason I'm not getting those anymore, so I'll put them back.
tests/dummy/config/coverage-babel.js
Outdated
@@ -0,0 +1,5 @@ | |||
/*jshint node:true*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jshint => eslint
tests/test-helper.js
Outdated
@@ -2,5 +2,7 @@ import resolver from './helpers/resolver'; | |||
import { | |||
setResolver | |||
} from 'ember-qunit'; | |||
// import { start } from 'ember-cli-qunit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this causing problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I recall, the tests didn't work with ember-cli-qunit@4.0.0-beta1. They apparently work with 4.0.0 now.
Put back more use strict, also reduce some of the overall change from the new ember-cli template.
@kellyselden OK all of the changes were made (the build failed intermittently again). Are there any other changes that need to be made? What else needs to be done to get this to be merged? I'm sure a lot of folks would like this support, and it's blocking other PRs like #119 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you remove the .bowerrc file as well?
after that it looks good to me
@@ -19,9 +20,6 @@ module.exports = { | |||
// JSHint "proto", disabled due to warnings | |||
'no-proto': 0, | |||
|
|||
// JSHint "strict" | |||
'strict': [2, 'global'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this necessary to remove now that the use strict issue is resolved?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I add that, then I get:
> ember-cli-code-coverage@0.3.12 lint /Users/paulw/Work/stuff/ember-cli-code-coverage
> eslint config lib test tests *.js
/Users/paulw/.../ember-cli-code-coverage/config/environment.js
2:1 error 'use strict' is unnecessary inside of modules strict
/Users/paulw/.../ember-cli-code-coverage/config/release.js
1:1 error 'use strict' is unnecessary inside of modules strict
/Users/paulw/.../ember-cli-code-coverage/lib/attach-middleware.js
1:1 error 'use strict' is unnecessary inside of modules strict
....
So which do you want, do you want the eslint rule or do you want the "use stricts"? Personally I'd rather have it hue closer to the default ember-cli blueprint, but that's just my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way you have it now is good
This ensures it works around ember-cli/ember-exam#81
I'd like someone a little closer to the codebase do the merge, but if no one does it soon I'll merge. |
Thanks @kellyselden |
I forgot to bring this up in the ember-cli meeting today. I'm sorry 😢 |
Is there anything I can do to help get this merged? 😄 |
Cutting release for this? |
[Fixes ember-cli-code-coverage#111] Update dependencies, get babel instrumenter working (ember-cli-code-coverage#115) babel 6 fix
Ping @kellyselden @rwjblue release? |
Released in v0.4.0. |
Yay! |
* Update CHANGELOG for 0.3.12. * 0.3.12 * Improve ES7 error message This messaging was confusing to me so I took a stab at clarifying the intent. * add mocha support and fix pretender bug * [Fixes #111] Update dependencies, get babel instrumenter working (#115) babel 6 fix * test * test * add mocha support and fix pretender bug [Fixes #111] Update dependencies, get babel instrumenter working (#115) babel 6 fix * 0.4.0 * update changelog v0.4.0 * Fixes the hanging issue of #88 (#90) * Refactor onload to onreadystatechange onreadystatechange is more reliable, chromium doesnt trigger onload * Fix PhantomJS honoring responseType issue * 0.4.1 * update changelog v0.4.1 * Suggest using `posttest` hook for `ember coverage-merge` It might be helpful to suggest how/where to run `coverage-merge` for people looking to set this up in a CI environment. The only other thing I might add is that you need to run `npm test` and have the appropriate configuration there for `posttest` to trigger, but I'm not sure if that's overkill since that should be common knowledge. * Resolve addon file paths correctly in CLI >= 2.12 * Spelling Fix * support nested coverageFolder * fix test * Update minimum version of ember-cli-babel. The previously locked version was not compatible with `node@8` (due to `engines` shenanigans). * Add babel-plugin-istanbul dep. * Use babel-plugin-istanbul instead of custom instrumenter. This has some negative effects still: * Does not re-write the paths to match "real" on disk paths * Does not instrument dummy app files (I think) Even with these negative side-effects, it has massive upside: * Massively less overall code to maintain * Does not require us to parse babel config (and therefore avoids issues around parallelism in broccoli-babel-transpiler) * Significantly faster when used (e.g. no longer has to double parse and process files) * Refactor middleware to use new istanbul API. * Refactor coverage-merge command to work with new istanbul-api. * Add node badge Figured it would be nice to have a badge for the node version, so people can quickly see what the latest release is. * 0.4.2 * Fix paths for istanbul report and remove parallel logic for impicit parallel support * Delete uneeded files * Remove component fixtures and add tests for 'excludes' config * Add support for in-repo-addons #120 * Revert parallel changes (TODO: move to another PR) * Only include test fixtures when testing the addon. * Add index.js unit tests * Update some docs * Add comment about .istanbul.yml to README * Fix typo * Bump ember, fix lint * Adjust some deps * Bump ember-cli-release * Add ember-cli-changelog * Released v1.0.0-beta.0 * Add back "Avoid throwing errors while requiring files for coverage" #64 See #63 Fixes #150 * Upgrade out-of-date deps * Update sinon * Setup travis ci to release on pushed tag, add lerna-changelog - Documented in RELEASE.md * Add v1.0.0-beta.1 to CHANGELOG [ci skip] * 1.0.0-beta.1 * Don't restrict travis to particular branches * Try using travis stages * Reformat .travis.yml and do not require sudo * Pin auto-dist-tag and add --no-sandbox to chrome args if on travis * Split out script for use with matrix build * Adjust deploy config * Make fixtures external (#156) * Start moving test files to separate addon * First pass at ember-cli-addon-tests * Fix test * Remove addon test, fix lint * Remove treeFor * Try sudo required * Remove no sandbox * Drop node 4 from travis, use npm instead of yarn * Add 8 * Add filter * Add testem.js to fixtures * Add eslint * Remove eslint plugins * Start in-repo addon tests (#158) * Start in-repo addon tests * Update per Adam's suggestion * Fix import paths * fix babel-plugin-istanbul caching issue (#159) * fix babel-plugin-istanbul caching issue * - Refactor instrumentation logic given `babel-plugin-istanbul` constraints - Fix Unit tests * Update app-coverage-test.js * Update in-repo-addon-coverage-test.js * Try setting path to process.cwd * Fix tests to workaround tomdale/ember-cli-addon-tests#176 * Update CHANGELOG for v1.0.0-beta.2. * 1.0.0-beta.2 * Support for addon-test-support (#160) * Start on support for addon-test-support * Remove only so all tests run * Try adding test-support prefix * Ensure addon-test-support coverage * Add tests for in-repo engines (#162) * First attempt at in-repo-engine * Fix engine coverage test * Fix lint * Update CHANGELOG for v1.0.0-beta.3. * 1.0.0-beta.3 * Remove merge-coverage and explicit parallel option (#163) * Remove merge-coverage and explicit parallel option * fix lint * Update README.md Describe how parallel works * Revert "Remove merge-coverage and explicit parallel option (#163)" This reverts commit 0592f5f. * Keep implicit and explicit parallel logic * Update babel-plugin-istanbul (#169) * Use the parent registry for determining JS extensions (#164) * Removing unused dependency exists-sync which fixes the deprecation warning from ember-cli (#179) * Update CHANGELOG for v1.0.0-beta.4. * 1.0.0-beta.4 * Fix fileLookup is null in testemMiddleware (#182) * upgrade istanbul-api to 2.0.1 (#186) istanbul-api@2.0.1 was released on June 6, 2018 and tagged as "next" on npm. The breaking change is pretty straightforward: https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-api/CHANGELOG.md#breaking-changes * Filter out in-repo addons that could not be found (#188) * Ember 3.4 (#190) * Bump deps (#191) * Bump deps * Reset engine test versions * Update CHANGELOG for v1.0.0-beta.5 * 1.0.0-beta.5 * Do not publish coverage, tests, or .idea to npm (#192) * Update babel-plugin-istanbul (#194) * Update CHANGELOG for v1.0.0-beta.6 * 1.0.0-beta.6 * TypeScript integration (howto) (#196) * TypeScript integration (howto) * removed some abstraction * 1.0.0-beta.6 * fix: handle babel 7 absolute paths (#199)
* Update CHANGELOG for 0.3.12. * 0.3.12 * Improve ES7 error message This messaging was confusing to me so I took a stab at clarifying the intent. * add mocha support and fix pretender bug * [Fixes ember-cli-code-coverage#111] Update dependencies, get babel instrumenter working (ember-cli-code-coverage#115) babel 6 fix * test * test * add mocha support and fix pretender bug [Fixes ember-cli-code-coverage#111] Update dependencies, get babel instrumenter working (ember-cli-code-coverage#115) babel 6 fix * 0.4.0 * update changelog v0.4.0 * Fixes the hanging issue of ember-cli-code-coverage#88 (ember-cli-code-coverage#90) * Refactor onload to onreadystatechange onreadystatechange is more reliable, chromium doesnt trigger onload * Fix PhantomJS honoring responseType issue * 0.4.1 * update changelog v0.4.1 * Suggest using `posttest` hook for `ember coverage-merge` It might be helpful to suggest how/where to run `coverage-merge` for people looking to set this up in a CI environment. The only other thing I might add is that you need to run `npm test` and have the appropriate configuration there for `posttest` to trigger, but I'm not sure if that's overkill since that should be common knowledge. * Resolve addon file paths correctly in CLI >= 2.12 * Spelling Fix * support nested coverageFolder * fix test * Update minimum version of ember-cli-babel. The previously locked version was not compatible with `node@8` (due to `engines` shenanigans). * Add babel-plugin-istanbul dep. * Use babel-plugin-istanbul instead of custom instrumenter. This has some negative effects still: * Does not re-write the paths to match "real" on disk paths * Does not instrument dummy app files (I think) Even with these negative side-effects, it has massive upside: * Massively less overall code to maintain * Does not require us to parse babel config (and therefore avoids issues around parallelism in broccoli-babel-transpiler) * Significantly faster when used (e.g. no longer has to double parse and process files) * Refactor middleware to use new istanbul API. * Refactor coverage-merge command to work with new istanbul-api. * Add node badge Figured it would be nice to have a badge for the node version, so people can quickly see what the latest release is. * 0.4.2 * Fix paths for istanbul report and remove parallel logic for impicit parallel support * Delete uneeded files * Remove component fixtures and add tests for 'excludes' config * Add support for in-repo-addons ember-cli-code-coverage#120 * Revert parallel changes (TODO: move to another PR) * Only include test fixtures when testing the addon. * Add index.js unit tests * Update some docs * Add comment about .istanbul.yml to README * Fix typo * Bump ember, fix lint * Adjust some deps * Bump ember-cli-release * Add ember-cli-changelog * Released v1.0.0-beta.0 * Add back "Avoid throwing errors while requiring files for coverage" ember-cli-code-coverage#64 See ember-cli-code-coverage#63 Fixes ember-cli-code-coverage#150 * Upgrade out-of-date deps * Update sinon * Setup travis ci to release on pushed tag, add lerna-changelog - Documented in RELEASE.md * Add v1.0.0-beta.1 to CHANGELOG [ci skip] * 1.0.0-beta.1 * Don't restrict travis to particular branches * Try using travis stages * Reformat .travis.yml and do not require sudo * Pin auto-dist-tag and add --no-sandbox to chrome args if on travis * Split out script for use with matrix build * Adjust deploy config * Make fixtures external (ember-cli-code-coverage#156) * Start moving test files to separate addon * First pass at ember-cli-addon-tests * Fix test * Remove addon test, fix lint * Remove treeFor * Try sudo required * Remove no sandbox * Drop node 4 from travis, use npm instead of yarn * Add 8 * Add filter * Add testem.js to fixtures * Add eslint * Remove eslint plugins * Start in-repo addon tests (ember-cli-code-coverage#158) * Start in-repo addon tests * Update per Adam's suggestion * Fix import paths * fix babel-plugin-istanbul caching issue (ember-cli-code-coverage#159) * fix babel-plugin-istanbul caching issue * - Refactor instrumentation logic given `babel-plugin-istanbul` constraints - Fix Unit tests * Update app-coverage-test.js * Update in-repo-addon-coverage-test.js * Try setting path to process.cwd * Fix tests to workaround tomdale/ember-cli-addon-tests#176 * Update CHANGELOG for v1.0.0-beta.2. * 1.0.0-beta.2 * Support for addon-test-support (ember-cli-code-coverage#160) * Start on support for addon-test-support * Remove only so all tests run * Try adding test-support prefix * Ensure addon-test-support coverage * Add tests for in-repo engines (ember-cli-code-coverage#162) * First attempt at in-repo-engine * Fix engine coverage test * Fix lint * Update CHANGELOG for v1.0.0-beta.3. * 1.0.0-beta.3 * Remove merge-coverage and explicit parallel option (ember-cli-code-coverage#163) * Remove merge-coverage and explicit parallel option * fix lint * Update README.md Describe how parallel works * Revert "Remove merge-coverage and explicit parallel option (ember-cli-code-coverage#163)" This reverts commit 0592f5f. * Keep implicit and explicit parallel logic * Update babel-plugin-istanbul (ember-cli-code-coverage#169) * Use the parent registry for determining JS extensions (ember-cli-code-coverage#164) * Removing unused dependency exists-sync which fixes the deprecation warning from ember-cli (ember-cli-code-coverage#179) * Update CHANGELOG for v1.0.0-beta.4. * 1.0.0-beta.4 * Fix fileLookup is null in testemMiddleware (ember-cli-code-coverage#182) * upgrade istanbul-api to 2.0.1 (ember-cli-code-coverage#186) istanbul-api@2.0.1 was released on June 6, 2018 and tagged as "next" on npm. The breaking change is pretty straightforward: https://github.com/istanbuljs/istanbuljs/blob/master/packages/istanbul-api/CHANGELOG.md#breaking-changes * Filter out in-repo addons that could not be found (ember-cli-code-coverage#188) * Ember 3.4 (ember-cli-code-coverage#190) * Bump deps (ember-cli-code-coverage#191) * Bump deps * Reset engine test versions * Update CHANGELOG for v1.0.0-beta.5 * 1.0.0-beta.5 * Do not publish coverage, tests, or .idea to npm (ember-cli-code-coverage#192) * Update babel-plugin-istanbul (ember-cli-code-coverage#194) * Update CHANGELOG for v1.0.0-beta.6 * 1.0.0-beta.6 * TypeScript integration (howto) (ember-cli-code-coverage#196) * TypeScript integration (howto) * removed some abstraction * 1.0.0-beta.6 * fix: handle babel 7 absolute paths (ember-cli-code-coverage#199)
This updates to the latest ember-cli and the latest blueprint. It also adds a yarn.lock file.
I also added an integration test for using the babel instrumenter and fixed it so the test passes. I've also been using ember-try to test against older LTS versions, and it seems to work.
Another addition might be to add ember-try support to travis.yml, and to update the other non-ember/non-babel dependencies.
Comments welcome, or suggestions for testing.
Still to do: