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

Remove line rate comparison for code coverage as that is not much useful due to comments #23389

Merged
merged 4 commits into from
Dec 23, 2024

Conversation

jatgarg
Copy link
Contributor

@jatgarg jatgarg commented Dec 20, 2024

Description

AB#23400
Based on the feedback and analysis we are removing line rate comparison for code coverage as that is not much useful due to comments increasing and decreasing it based on whether the comments was in covered code or non-covered code.

@jatgarg jatgarg requested a review from alexvy86 December 20, 2024 20:03
@jatgarg jatgarg self-assigned this Dec 20, 2024
@Copilot Copilot bot review requested due to automatic review settings December 20, 2024 20:03
@github-actions github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels Dec 20, 2024

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Copy link
Contributor

@alexvy86 alexvy86 left a comment

Choose a reason for hiding this comment

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

This bit looks fine. I suggest that we either remove the properties related to line coverage from CodeCoverageComparison or add notes to them saying we deliberately don't use them.

@github-actions github-actions bot added the area: loader Loader related issues label Dec 20, 2024
Copy link
Contributor Author

@jatgarg jatgarg left a comment

Choose a reason for hiding this comment

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

Code Coverage Summary

No packages impacted by the change.


Baseline commit: 9bdb9b5
Baseline build: 313948
Happy Coding!!

Code coverage comparison check passed!!

Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

How is coverage reported for type only files with this change? They have 1 branch that is never covered? That happens all the time or some?
Definitely forward progress - just wondering what to expect.

packages/loader/container-loader/src/container.ts Outdated Show resolved Hide resolved
packages/loader/container-loader/src/utils.ts Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the area: loader Loader related issues label Dec 23, 2024
@jatgarg
Copy link
Contributor Author

jatgarg commented Dec 23, 2024

How is coverage reported for type only files with this change? They have 1 branch that is never covered? That happens all the time or some? Definitely forward progress - just wondering what to expect.

Removed line coverage in order to prevent that. From what I noticed, the types or declaration will not be counted a branch and therefore, that would not affect the code coverage. We will plan to add method coverage as well alongside branch coverage to make it more robust.

@jatgarg jatgarg merged commit f6bf3a8 into microsoft:main Dec 23, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants