feat(test): Add code coverage for karma runs #895
Conversation
39f3d3a to
c9f43fa
Compare
|
I think my accidental closing of the issue caused this to not build on AppVeyor. Is there a way rerun the build manually? |
| "karma-jasmine": "^0.3.8", | ||
| "karma-remap-istanbul": "git+https://github.com/marines/karma-remap-istanbul.git#patch-1", |
There was a problem hiding this comment.
I'm sorry but I can't allow every CLI dependencies to be reliant on a ref on someone's GitHub.
There was a problem hiding this comment.
I agree. So, there are two options:
- We wait for the dev to implement the PR for
karma-remap-istanbul - We/I/Someone creates
karma-remap-istanbul2with the fix.
Suggestions?
c9f43fa to
79470a3
Compare
|
@hansl, I got in contact with @marcules, the maintainer for The only concern I have left is the fact everything is considered to be under Let me know if there is anything else you would like me fix! Thanks. |
|
Found another concern, maybe someone can help. I am getting |
|
@delasteve can you tell me how to repro that error? |
79470a3 to
dd08c48
Compare
|
@filipesilva: it's an issue with I just released a version that will fix the timeout issue that was causing that error line. It was always executing the timeout even if all the files were found. I just fixed the cli |
|
@filipesilva would you mind rerunning the build? AppVeyor looks like it had a network issue and while the build completed and tests were the same I had on my local, the TravisCI also failed. Thanks! |
|
@delasteve I restarted both builds now. |
|
I will take a look into the failures when I get home in a bit. Curious if it has something to do with my update to the |
|
@delasteve @filipesilva I tried the same approach as you and had the issue that the html report was not generated on the first run. Do you have the same issue? I have another approach that also removes the dependency to karma-remap-istanbul, you can check it here: https://github.com/devCrossNet/angular-cli/tree/coverage. I also made a pull-request to the remap-istanbul project that should solve the "missing folder structure" issue, you can see it here: SitePen/remap-istanbul#65. My approach will work after the pull-request is merged and I update the remap-istanbul dependecy. Let me know what you think. |
|
@filipesilva I took a look locally, and it passes there. I don't know what's going on with CI. @devCrossNet I like your approach towards th
8000
e solution. I am not a fan of I wonder if we go the route of a new task, should we have a |
|
@delasteve I think coverage should be the default option. but with my solution, it is possible to implement a --coverage flag. |
|
@devCrossNet I agree about it being default. At one point, Also, I like your solution to this better than using the flakey karma plugin. How should we go about this? I would rather not just lift your code and put my name on the commit. Thoughts? |
|
@delasteve I am really new into contributing to projects on GitHub. I have no idea. Maybe you can merge my changes with yours? Then it should be a PR from us both |
|
@delasteve to be perfectly honest I don't know why travis is failing. The tests all pass and then it seems to hang. Perhaps there is a zombie process created by the new libs that isn't killed? |
|
@filipesilva I almost want to say forget my PR and go with @devCrossNet's solution. It's been running for over 2 hours at this point on Travis. I am not found of the solution @devCrossNet: I am pretty sure the Angular team would like 1 commit per PR. Git only allows 1 author per PR. Personally, I would go ahead and rebase your changes on top of |
|
@delasteve ok, as soon as my changes are merged into remap-istanbul and I finished this whole topic. I will submit a new PR for this feature. |
|
@delasteve I updated my repository to use my fork from remap-istanbul. In my opinion, this can be a temporary solution for this feature. But I have another question - should the code coverage also work with ng test --watch=true? |
|
@devCrossNet Got in touch with someone from SitePen. They will be taking a look at your PR in a bit. As for running with |
|
Don't worry too much about the number of commit guys, once it's ready to merge in I'll just squash them myself. Regarding the watch, I agree that it should get coverage as well by default. If you implement a |
|
@delasteve @filipesilva thank you very much, guys. I will try to implement the coverage with the watch=true flag. I have no idea how to do that because the karma-server event "run_completed" is triggered before the coverage-final.json is written. maybe I have to set a timeout or something. |
|
@delasteve Now it works for ng test and ng test--watch=false. It is really fast and the usage feels good. It would be great if you can do a little review and maybe test it. devCrossNet@a7b3ebe |
|
@devCrossNet it appears that there is an issue with your pull request for After that goes in, I definitely will be able to test your changes. |
|
@delasteve sorry for the delay. I was sick the last view days. I have to figure out what is the problem with remap-istanbul. Maybe the solution is just to test the values. I will ask him to give a reproducible example. It would be great if you can help me if you know the remap-istanbul project. |
dd08c48 to
39dc1cb
Compare
39dc1cb to
f4c23ab
Compare
|
@delasteve remap-istanbul merged my changes today. after they released the new npm version, I will update my code and make a PR. regards. |
|
Closed as this PR was made obsolete by #1455. Code coverage is in that PR though! |
|
I'm still facing an issue when I implemented Karma-Coverage in my Angular-CLI project. I’m not able to see the correct file names and contents in the HTML report. The coverage is fine but, the file location and name is messed up, it looks like it’s being picked up from the ‘tmp’ folder (tmp/broccoli_type_script_compiler-input_base_path-xrfIqZiR.tmp/0/src/app/). The remapping may have gone wrong. Any workaround or a fix too see the correct file contents within the report. Thanks in advance. |
|
@Headcult try the webpack preview build, that should be fixed: https://github.com/angular/angular-cli#webpack-preview-release-update |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.