chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 for coverage excludes #4195

dpmorrow · 2017-01-24T07:56:39Z

Implemented coverage excludes as an array of blobs and/or paths relative to the app root.

See README.md for documentation.

Fulfills #3356

googlebot · 2017-01-24T07:56:40Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

dpmorrow · 2017-01-24T09:03:15Z

I signed it!

googlebot · 2017-01-24T09:03:16Z

CLAs look good, thanks!

filipesilva · 2017-01-24T11:03:17Z

Heya @dpmorrow, thanks for taking the time to do this PR. It's well done and kudos for including the test case as well!

If possible, I'd prefer to not add more options to angular-cli.json though. Have you tried the exclude option as described here marcules/karma-remap-istanbul#26?

To me it would be preferable to use it instead, if it covers your usecase.

hansl · 2017-01-24T17:54:46Z

tests/e2e/utils/fs.ts

@@ -138,6 +138,29 @@ export function expectFileToMatch(fileName: string, regEx: RegExp | string) {
    });
 }

+export function expectFileNotToMatch(fileName: string, regEx: RegExp | string) {


Why not expectToFail(() => expectToMatch(fileName, regEx))?

@hansl I agree, I read some more tests while waiting for Travis and realized that was probably the right way to work with existing helpers.

hansl · 2017-01-24T17:55:54Z

packages/angular-cli/lib/config/schema.json

@@ -61,6 +61,12 @@
          "test": {
            "type": "string"
          },
+          "coverageExcludes": {


Could you give me an example of code (outside of specs and node_modules) you want to exclude?

I think a smarter default to exclude everything outside of the appRoot as well would be better.

For example, appRoot/testing/test.helper.ts. I understand as a workaround I could've used test.helper.spec.ts but that was too messy IMO.

I'm going to check out the karma-remap-istanbul 0.3.0 fix that @felipesilva suggested when I get home later. The only difference there being no glob support, but that could be implemented on the karma-remap-istanbul side if really necessary (and it probably isn't).

So at any rate I'll check out that solution and probably recommend closing this in favor of updating to ^0.3.0 karma-remap-istanbul and adding documentation to guide people to that solution.

If you have strong feelings one way or another let me know, otherwise thanks to both of you for the input!

I only wonder why I couldn't find @felipesilva's solution when I looked! Oh well, this was a good and fun learning experience regardless!

dpmorrow · 2017-01-25T05:35:45Z

I confirmed that my use case is resolved by 0.3.0 karma remap.

Now I don't think I've got the energy to complete the effort tonight, however this is what I'm thinking:

update package.json blueprint to karma-remap-istanbul:^0.3.0
- preliminary e2e test run shows no issues with that
update karma.conf.js blueprint to use karma-remap-istanbul exclude syntax
- use /(?!<%= sourceDir %>)/ to implement @hansl's suggestion
- move the spec/e2e ignore here as well
- this would have the side effect of both demonstrating the functionality as well as making the tool just that much more flexible.
remove vestigial exclude from getWebpackTestConfig as this would be restricting the aforementioned flexibility.
have the test at least check for the spec blueprint being ignored.
if the test runner can handle it, create a file one level above appRoot and check that it's not being covered.
- this depends on whether the cleanup between tests can handle that (haven't read that far)
- also seems like you would have to get the file included in the test run
  - I guess that would be: read the contents of app.component.spec.ts and concatenate an import

Does that seem about right? Would the test runner clean something up in the directory above app?

I guess I could also delete the file after the expectation is done if not.

Would you think this would need a call out in the readme anymore or just let the karma.conf.js that's generated do the explaining in conjunction with karma-remap-istanbul docs?

dpmorrow · 2017-01-25T20:10:16Z

I was thinking about it and realized that removing the exclude being sent to the instrumentation loader would be a breaking change for existing projects. That is to say they would need to port their project to using the karma-remap-istanbul exclude if I do the change as I suggested above.

Will that be an issue?

dpmorrow · 2017-01-26T06:48:35Z

I guess I messed up my rebase broke this PR. I decided to just update the dependency version and put a note in the readme pointing to the issue in karma-remap-istanbul. I will submit a new PR with just that and reference this one.

Edit: just pushed new changes and reopened.

dpmorrow · 2017-01-28T19:06:30Z

The build failed due to linting but doesn't fail for me locally... Not sure what's wrong here but I'm going to rebase it again and see if it succeeds.

…for coverage excludes Update the project blueprint to allow specifying additional coverage excludes via the karma- istanbul-remap excludes property that was added in 0.3.0.

dpmorrow · 2017-02-05T01:11:42Z

@hansl simple blueprint/readme update ready to merge.

filipesilva · 2017-02-10T09:52:44Z

I'm sorry for this, but we had to move away from remap-instanbul due to licensing concerns. You can see more about it in #4560.

dpmorrow · 2017-02-10T21:18:20Z

@filipesilva thanks for the heads up on this. I'll watch the progress on the new reporter and see if it's appropriate to do a new PR in the future when and if the maintainer adds the equivalent functionality back.

Take care and keep up the great work!

angular-automatic-lock-bot · 2019-09-11T20:41:24Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: no label Jan 24, 2017

googlebot added cla: yes and removed cla: no labels Jan 24, 2017

filipesilva added pr_action: review and removed pr_action: review labels Jan 24, 2017

hansl suggested changes Jan 24, 2017

View reviewed changes

dpmorrow closed this Jan 26, 2017

dpmorrow force-pushed the coverage-excludes branch from f2fdc6e to 790dda6 Compare January 26, 2017 06:45

dpmorrow reopened this Jan 26, 2017

dpmorrow force-pushed the coverage-excludes branch 4 times, most recently from 39e5a99 to ee5d7f5 Compare February 3, 2017 04:36

dpmorrow changed the title ~~feat(test): exclude files from coverage~~ chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 for coverage excludes Feb 3, 2017

chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 …

917f2d9

…for coverage excludes Update the project blueprint to allow specifying additional coverage excludes via the karma- istanbul-remap excludes property that was added in 0.3.0.

dpmorrow force-pushed the coverage-excludes branch from ee5d7f5 to 917f2d9 Compare February 5, 2017 00:33

filipesilva closed this Feb 10, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 for coverage excludes #4195

chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 for coverage excludes #4195

dpmorrow commented Jan 24, 2017

googlebot commented Jan 24, 2017

dpmorrow commented Jan 24, 2017

googlebot commented Jan 24, 2017

filipesilva commented Jan 24, 2017

hansl Jan 24, 2017

dpmorrow Jan 24, 2017

hansl Jan 24, 2017

dpmorrow Jan 24, 2017

dpmorrow commented Jan 25, 2017 •

edited

Loading

dpmorrow commented Jan 25, 2017

dpmorrow commented Jan 26, 2017 •

edited

Loading

dpmorrow commented Jan 28, 2017

dpmorrow commented Feb 5, 2017

filipesilva commented Feb 10, 2017

dpmorrow commented Feb 10, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 for coverage excludes #4195

chore(test): update project blueprint to karma-remap-istanbul ^0.3.0 for coverage excludes #4195

Conversation

dpmorrow commented Jan 24, 2017

googlebot commented Jan 24, 2017

dpmorrow commented Jan 24, 2017

googlebot commented Jan 24, 2017

filipesilva commented Jan 24, 2017

hansl Jan 24, 2017

Choose a reason for hiding this comment

dpmorrow Jan 24, 2017

Choose a reason for hiding this comment

hansl Jan 24, 2017

Choose a reason for hiding this comment

dpmorrow Jan 24, 2017

Choose a reason for hiding this comment

dpmorrow commented Jan 25, 2017 • edited Loading

dpmorrow commented Jan 25, 2017

dpmorrow commented Jan 26, 2017 • edited Loading

dpmorrow commented Jan 28, 2017

dpmorrow commented Feb 5, 2017

filipesilva commented Feb 10, 2017

dpmorrow commented Feb 10, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

dpmorrow commented Jan 25, 2017 •

edited

Loading

dpmorrow commented Jan 26, 2017 •

edited

Loading