Remove merge-coverage and explicit parallel option #163

adamjmcgrath · 2018-02-14T19:43:07Z

The goal of this PR is to simplify measuring coverage when parallel testing, buy removing the merge-coverage command and explicit parallel options.

Instead, we can create a coverageMap once when we attach the middleware then merge the coverage data into it after each parallel test run.

For the ember serve case, we would continue to create a new coverage map on every test run.

RobbieTheWagner · 2018-02-20T16:41:42Z

@adamjmcgrath so can you please explain this further? Does this essentially mean we can run COVERAGE=true ember test or COVERAGE=true ember exam and it just works magically with both cases? Will it work fine with Travis and other CI?

adamjmcgrath · 2018-02-20T16:46:01Z

Yep - exactly, this test should confirm that: https://github.com/kategengler/ember-cli-code-coverage/blob/a973cfe2f003aa8ae7e023ad6f3de61de4ba1aca/test/integration/app-coverage-test.js#L79-L88

RobbieTheWagner · 2018-02-20T16:55:34Z

@adamjmcgrath Nice! Will there be any performance issues with people running just plain ember t? We should probably document that parallel testing can now just be done with COVERAGE=true ember exam etc. That way, it is obvious it is supported.

adamjmcgrath · 2018-02-20T17:14:50Z

Will there be any performance issues with people running just plain ember t?

No, the coverage merging logic is in the middleware which only get's attached when coverage is enabled

We should probably document that parallel testing can now just be done with COVERAGE=true ember exam etc. That way, it is obvious it is supported.

👍 will update the README

RobbieTheWagner · 2018-02-20T17:40:30Z

@adamjmcgrath

No, the coverage merging logic is in the middleware which only get's attached when coverage is enabled

I mean when coverage is enabled. Will people running COVERAGE=true ember t experience any slowdowns or anything different than before? I'm basically wondering if we want to always run this stuff or if we should still pass a flag or check for ember exam before merging stuff.

adamjmcgrath · 2018-02-20T23:33:52Z

Will people running COVERAGE=true ember t experience any slowdowns or anything different than before?

COVERAGE=true ember t wont be affected at all - the coverage map will still be created once and the report will still be generated once.

Describe how parallel works

RobbieTheWagner

This seems good to me. Thanks for all the hard work!

kategengler · 2018-02-21T03:58:18Z

I believe this will remove a bit of functionality: If you run ember exam partitioned (say in separate CI containers), you can no longer merge coverage into one report (https://github.com/trentmwillis/ember-exam#split-test-parallelization).

RobbieTheWagner · 2018-02-21T04:32:06Z

@kategengler I asked if this should work with CI still, and I thought @adamjmcgrath confirmed it would?

kategengler · 2018-02-21T04:38:52Z

I believe it will work fine on CI if you're not using the partition option for ember exam.

RobbieTheWagner · 2018-02-21T04:49:03Z

@adamjmcgrath can you confirm if this will work with partition? I think you had said you were running with partition for your company's test suite right?

adamjmcgrath · 2018-02-21T07:25:20Z

I believe this will remove a bit of functionality: If you run ember exam partitioned (say in separate CI containers), you can no longer merge coverage into one report

Ah yep - I didn't consider this use case. I use the parallel feature of ember exam but not partition. We could put the coverage-merge command back although perhaps recommending something like istanbul-merge (or possibly nyc when istanbuljs/nyc/issues/515 is complete) would be simpler?

RobbieTheWagner · 2018-02-21T13:31:42Z

@adamjmcgrath I'll let you and @kategengler decide on that 😃. I don't use ember exam myself, so I thought parallel was the only thing we needed and that ran on multiple boxes, but I suppose I was confusing it with partition.

This reverts commit 0592f5f.

)"" This reverts commit 2a6ccb4.

Revert "Remove merge-coverage and explicit parallel option (#163)"

* 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)

adamjmcgrath added 2 commits February 14, 2018 19:26

Remove merge-coverage and explicit parallel option

8f87004

fix lint

a973cfe

Update README.md

5b0dd86

Describe how parallel works

RobbieTheWagner requested review from kategengler and removed request for kategengler February 21, 2018 03:36

RobbieTheWagner approved these changes Feb 21, 2018

View reviewed changes

RobbieTheWagner added the enhancement label Feb 21, 2018

RobbieTheWagner merged commit 0592f5f into master Feb 21, 2018

RobbieTheWagner deleted the parallel-refactor branch February 21, 2018 03:39

adamjmcgrath added a commit that referenced this pull request Feb 27, 2018

Revert "Remove merge-coverage and explicit parallel option (#163)"

2a6ccb4

This reverts commit 0592f5f.

adamjmcgrath added a commit that referenced this pull request Feb 27, 2018

Revert "Revert "Remove merge-coverage and explicit parallel option (#163

49d2fd2

)"" This reverts commit 2a6ccb4.

adamjmcgrath mentioned this pull request Feb 27, 2018

Revert "Remove merge-coverage and explicit parallel option (#163)" #166

Merged

kategengler added a commit that referenced this pull request Mar 24, 2018

Merge pull request #166 from kategengler/revert-pr-163

d234882

Revert "Remove merge-coverage and explicit parallel option (#163)"

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove merge-coverage and explicit parallel option #163

Remove merge-coverage and explicit parallel option #163

adamjmcgrath commented Feb 14, 2018

RobbieTheWagner commented Feb 20, 2018 •

edited

Loading

adamjmcgrath commented Feb 20, 2018

RobbieTheWagner commented Feb 20, 2018

adamjmcgrath commented Feb 20, 2018

RobbieTheWagner commented Feb 20, 2018

adamjmcgrath commented Feb 20, 2018

RobbieTheWagner left a comment

kategengler commented Feb 21, 2018

RobbieTheWagner commented Feb 21, 2018

kategengler commented Feb 21, 2018

RobbieTheWagner commented Feb 21, 2018

adamjmcgrath commented Feb 21, 2018

RobbieTheWagner commented Feb 21, 2018

Remove merge-coverage and explicit parallel option #163

Remove merge-coverage and explicit parallel option #163

Conversation

adamjmcgrath commented Feb 14, 2018

RobbieTheWagner commented Feb 20, 2018 • edited Loading

adamjmcgrath commented Feb 20, 2018

RobbieTheWagner commented Feb 20, 2018

adamjmcgrath commented Feb 20, 2018

RobbieTheWagner commented Feb 20, 2018

adamjmcgrath commented Feb 20, 2018

RobbieTheWagner left a comment

Choose a reason for hiding this comment

kategengler commented Feb 21, 2018

RobbieTheWagner commented Feb 21, 2018

kategengler commented Feb 21, 2018

RobbieTheWagner commented Feb 21, 2018

adamjmcgrath commented Feb 21, 2018

RobbieTheWagner commented Feb 21, 2018

RobbieTheWagner commented Feb 20, 2018 •

edited

Loading