Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat(@angular-devkit/build-angular): karma-coverage w/ app builder #28509

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

Merged
jkrems merged 1 commit into angular:main from jkrems:jk-ill-cover-you
Sep 27, 2024

Conversation

Copy link
Contributor

@jkrems jkrems commented Sep 26, 2024
edited
Loading

Tasks to Cover (πŸ₯)

  • Determine list/filtering of files to instrument for coverage.
  • Actually instrument those files. Importantly, using a recent version of istanbul-lib-instrument and not the one that karma-coverage depends on.
  • Preserve source maps so that coverage is mapped back to source files.

Implementation Notes

  • The additions in packages/angular/build/src/tools/babel/plugins/* are 1:1 copies of files already present in packages/angular_devkit/build_angular/src/tools/babel/plugins/*. I couldn't find a clean way to prevent that. Either @angular/build have to expose the plugin in package.json#exports or it would have to depend on @angular-devkit/build-angular. Both seemed undesirable.
  • The logic for excluding files from coverage is lifted from the equivalent webpack config. Given the state of Karma, I didn't make an effort to refactor the code to share the logic - I assume we don't expect many changes here. That would definitely be possible with limited effort though.

Historical Notes

Approaches that didn't work:

  • Instrument entire output chunks. karma-coverage really doesn't want to filter data from coverage data after the fact. Not to mention that it could be fairly expensive to send that data back to the karma process if it's then just discarded.
  • Implied above: Use karma-coverage's built-in preprocessor. The outdated instrumentation code it's using doesn't handle async function correctly and gives bad function coverage data because of it.

Things that could've worked:

  • Apply source maps while instrumenting instead of while post-processing the coverage data. But that would've been swimming against the stream.

@jkrems jkrems changed the title (ε‰Šι™€) Jk-ill-cover-you (ε‰Šι™€γ“γ“γΎγ§) (追記) feat(@angular-devkit/build-angular): karma-coverage w/ app builder (θΏ½θ¨˜γ“γ“γΎγ§) Sep 26, 2024
@jkrems jkrems force-pushed the jk-ill-cover-you branch 2 times, most recently from 13840aa to 00bb941 Compare September 27, 2024 15:02
@jkrems jkrems marked this pull request as ready for review September 27, 2024 15:11
@jkrems jkrems requested a review from clydin September 27, 2024 15:11
@jkrems jkrems added target: feature This PR is targeted for a feature branch (outside of main and semver branches) target: minor This PR is targeted for the next minor release action: review The PR is still awaiting reviews from at least one requested reviewer and removed target: feature This PR is targeted for a feature branch (outside of main and semver branches) labels Sep 27, 2024
@jkrems jkrems added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 27, 2024
@jkrems jkrems merged commit 0a4ef30 into angular:main Sep 27, 2024
31 checks passed
@jkrems jkrems deleted the jk-ill-cover-you branch September 27, 2024 19:06
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clydin clydin clydin approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: @angular-devkit/build-angular detected: feature PR contains a feature commit target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

2 participants

AltStyle γ«γ‚ˆγ£γ¦ε€‰ζ›γ•γ‚ŒγŸγƒšγƒΌγ‚Έ (->γ‚ͺγƒͺγ‚ΈγƒŠγƒ«) /