Skip to content
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

feat(@angular-devkit/build-angular): Switch to karma-coverage #17901

Merged
merged 1 commit into from
Sep 6, 2020

Conversation

kyliau
Copy link
Contributor

@kyliau kyliau commented Jun 10, 2020

This commit switches coverage tooling from karma-coverage-istanbul-reporter
to karma-coverage since it's better supported.

Fixes #17757

@kyliau kyliau force-pushed the karma-coverage branch 2 times, most recently from 7a20487 to 6de7228 Compare June 10, 2020 23:04
@kyliau kyliau force-pushed the karma-coverage branch 8 times, most recently from feb9f02 to a7fef26 Compare July 1, 2020 04:28
@kyliau kyliau changed the title feat(build-angular): Switch to karma-coverage feat(@angular-devkit/build-angular): Switch to karma-coverage Jul 1, 2020
@kyliau kyliau marked this pull request as ready for review July 1, 2020 17:11
@kyliau
Copy link
Contributor Author

kyliau commented Jul 1, 2020

Because the coverage test is run asynchronously in a "runaway" promise, I had to do two things here to make the tests work:

  1. Call setTimeout to wait for coverage files to be written
  2. Pass Karma results to the callback because the callback is immediately invoked even though the test hasn't actually completed. Passing the Karma results by reference allows us to retrieve the final exit code modified by karma-coverage if the coverage doesn't meet the threshold.

I'm not very satisfied with both approaches, but after discussing with @filipesilva I think this is the best compromise.
I'm totally happy to revise this PR if there are better suggestions.

@kyliau kyliau added the target: major This PR is targeted for the next major release label Jul 1, 2020
expect(result.success).toBeTrue();
const karmaResults: TestResults = result.karmaResults as never;
// exitCode is non-zero because coverage failed
expect(karmaResults.exitCode).toBe(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change, it means that currently with the CLI, there is no way to exit the process with a non zero error code when code coverage threshold is not met.

Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

Left a couple of comments.

Also, currently code-coverage in the console we display a summary, however it seems that it doesn't work with the new plugin. Can you take a look?

TOTAL: 3 SUCCESS
TOTAL: 3 SUCCESS
TOTAL: 3 SUCCESS

=============================== Coverage summary ===============================
Statements   : 88.89% ( 8/9 )
Branches     : 100% ( 0/0 )
Functions    : 100% ( 0/0 )
Lines        : 85.71% ( 6/7 )
================================================================================

@kyliau kyliau force-pushed the karma-coverage branch 3 times, most recently from 6077037 to a33b3a5 Compare July 20, 2020 20:40
@kyliau
Copy link
Contributor Author

kyliau commented Jul 29, 2020

Update: even though karma-coverage 2.0.3 was released to address the issue of onRunComplete() return a promise, more changes are needed upstream before this could be implemented on CLI side without hacky solutions. This is currently blocked on:

  1. fix(reporter): set exit code when thresholds aren't met karma-runner/karma-coverage#420
  2. feat(server): allow 'exit' listeners to set exit code karma-runner/karma#3541

Since karma-coverage 2.0.3 no longer modifies the karma results in-place, the solution implemented in this PR is no longer applicable.

@alan-agius4
Copy link
Collaborator

@kyliau, any chance to get in touch with one of the Karma Googler’s to prioritise the above fixes?

@kyliau kyliau force-pushed the karma-coverage branch 7 times, most recently from f69bef7 to da462ea Compare September 4, 2020 02:21
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@alan-agius4
Copy link
Collaborator

@googlebot I consent

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Sep 4, 2020
Copy link
Collaborator

@alan-agius4 alan-agius4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one small NIT.

@kyliau kyliau force-pushed the karma-coverage branch 2 times, most recently from 09ccaf4 to 4d89628 Compare September 5, 2020 05:54
This commit switches coverage tooling from karma-coverage-istanbul-reporter
to karma-coverage since it's better supported.

Closes angular#17757
@alan-agius4 alan-agius4 added the action: merge The PR is ready for merge by the caretaker label Sep 6, 2020
@alan-agius4 alan-agius4 merged commit 8995e49 into angular:master Sep 6, 2020
@kyliau kyliau deleted the karma-coverage branch September 8, 2020 17:16
@angular-automatic-lock-bot
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 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

karma-coverage-istanbul-reporter author requires payment for support and issue reporting
4 participants