Testing all WinForms controls have unit tests coverage #10476

SimonZhao888 · 2023-12-14T03:12:21Z

Fixes #10453

Proposed changes

Enable code coverage on public PR builds.

Customer Impact

No

Regression?

No

Risk

-Yes

Screenshots

Test methodology

Pipeline link Testing all WinForms controls have unit tests coverage

Microsoft Reviewers: Open in CodeFlow

Tanya-Solyanik · 2023-12-15T22:27:09Z

@sharwell - do you have any recommendations on how to re-enable code coverage, or sample repos we could take a look at?

RussKie · 2023-12-17T22:49:50Z

I've done this in a few repo using https://github.com/microsoft/codecoverage/. E.g., dotnet/aspire#569.
Please ping me if you have any questions or need help.

sharwell · 2023-12-18T17:23:21Z

My recommendation would be to follow https://github.com/dotnet/roslyn-analyzers/, using coverlet, ReportGenerator, and codecov.io. At this time I cannot recommend the use of microsoft/codecoverage due to its licensing policy and lack of benefits over the more permissive/open/well-established packages which I recommend instead.

RussKie · 2023-12-19T09:58:47Z

My recommendation would be to follow dotnet/roslyn-analyzers, using coverlet, ReportGenerator, and codecov.io. At this time I cannot recommend the use of microsoft/codecoverage due to its licensing policy and lack of benefits over the more permissive/open/well-established packages which I recommend instead.

@sharwell why are you suggesting to use 3P service (i.e., codecov.io) over using the MS services? This implies sending data to 3P.
Also, what is the problem with microsoft/codecoverage licensing?

/cc: @jakubch1

sharwell · 2023-12-19T14:31:23Z

why are you suggesting to use 3P service (i.e., codecov.io) over using the MS services?

I haven't seen any 1P service that works in this space. Specifically, all 1P services that I've observed focus on the code coverage for the repository as a whole, a largely irrelevant feature. The single highest-value code coverage feature which any system must implement to even be considered viable is allowing a pull request reviewer to see the code coverage impact of that pull request. This includes the following two items:

(Most important) Show the code coverage of the lines of code changed by that pull request
(Secondary) Show the changes to code coverage occurring on lines of code not touched by that pull request

This behavior aids code reviewers in asking smart questions about complex sections of code which are being reviewed, such as seeing scenario tests that cover special cases, or asking why a special case in code isn't being covered. Special cases in code are more likely to be the source of a regression because they are less likely to be hit in the common case.

Also, what is the problem with microsoft/codecoverage licensing?

It's not open source, but there are other open-source alternatives which have been around longer and perform just as well. I can't condone choosing a closed ecosystem without clear benefits.

eng/MergeCoverageData.ps1

src/Microsoft.VisualBasic.Forms/tests/UnitTests/Microsoft.VisualBasic.Forms.Tests.vbproj

eng/pipelines/build.yml

eng/MergeCoverageData.ps1

sharwell · 2023-12-28T14:07:29Z

The removal of codecov.io upload is highly unfortunate. I've found the Azure Pipelines code coverage features lacking to the point of basically useless. I have no objection to publishing in both locations, but if the data only goes to one location it should be codecov.io.

SimonZhao888 · 2024-01-02T02:46:45Z

but if the data only goes to one location it should be codecov.io.

OK, I've added this task back.

Tanya-Solyanik · 2024-01-02T21:40:51Z

@SimonZhao888 - Would it make sense to split this PR into 2 PRs or 2 commits. First do a cleanup and refactoring tasks, and then enable the code coverage? It will be easier to review and maintain this way. This is what Sam did with his big PR - #10552

sharwell · 2024-01-02T21:49:55Z

I may be able to help with it as well once the integration test changes are completed.

eng/MergeCoverageData.ps1

Tanya-Solyanik · 2024-01-02T21:45:14Z

eng/pipelines/build.yml

+      condition: and(eq(variables['_BuildConfig'], 'Debug'), eq('${{ parameters.targetArchitecture }}', 'x64'))
+
+    #Publish code coverage results
+    - task: PublishCodeCoverageResults@1


We have PR and CI builds in GH, and in the internal AzDO repo. Where is this task executed?

Since PublishCodeCoverageResults@1 is an AzDO internal task, I assume it will be executed at AzDO.

I can't tell if this is asking about GitHub Actions vs Azure Pipelines, or if it's asking about Azure Pipelines public build vs Azure Pipelines internal/signed build. I do know this file is only active in Azure Pipelines builds, but not sure if it's running in the internal/signed build.

Hi @sharwell,

Could you please help me with a question?
Now we are going to count the code coverage of all the automated tests, I am checking the coverage results on CodeCov server, but found that the results counted on CodeCov server are much different from the results on Azure pipeline, I am not sure if I am missing any settings, Could you please point them out for me?

Pipeline link: codecoverage-tab

CodeCov link: CodeCov-codecoverage

Now we are going to count the code coverage of all the automated tests

Yes, this is hugely beneficial in identifying certain categories of accidental bugs and/or behavior changes.

I am checking the coverage results on CodeCov server, but found that the results counted on CodeCov server are much different from the results on Azure pipeline

I noticed the same. I was planning to make some tweaks either after you are ready for final review/merge or after it merges. Once you reach a stable point I can go over those details with you.

Items I'm planning to verify later (could be part of this PR or separate):

Ensure we only upload exactly one coverage report to codecov.io for a single PR submission. Historically (talking 2+ years ago), codecov.io internally was not efficient at merging multiple reports, which could cause problems for large repositories. I found that merging the reports before submitting them significantly improved response times and reliability.

Review behavior for production code (make sure it appears, check status for flags)

Review behavior for test code (make sure it appears)

Review behavior for build-time generated code, see if there is any way to make it appear in tools

Make sure the codecov.io comment is appearing in PRs. It's already configured to appear but hasn't in this PR for some unknown reason.

The first item is the most important. During my work for this item, I'll be comparing the behavior of WinForms with the behavior of dotnet/roslyn-analyzers (uses coverlet+Arcade which we want to match) and DotNetAnalyzers/StyleCopAnalyzers (uses OpenCover but has a proper Publish Code Coverage stage that I can model after if we need it).

eng/CodeCoverage.proj

eng/pipelines/build.yml

...mitives/tests/TestUtilities.Tests/System.Windows.Forms.Primitives.TestUtilities.Tests.csproj

Tanya-Solyanik · 2024-01-08T21:29:03Z

Will a diff between coverage before the PR and coverage after the PR be published when PR build completes?

sharwell · 2024-01-08T21:31:56Z

Will a diff between coverage before the PR and coverage after the PR be published when PR build completes?

That is the expectation. It's not clear yet why it's not showing up in this repository, but you can see it in action here:
dotnet/roslyn-analyzers#7116

At the time of this comment, it says there are 38 lines changed in my linked PR which are not fully covered, out of a total of around 400 LOC touched by the PR.

SimonZhao888 · 2024-01-09T01:02:17Z

That is the expectation. It's not clear yet why it's not showing up in this repository, but you can see it in action here:
dotnet/roslyn-analyzers#7116

I'll double-check the differences in these two pipelines to enable the feature in codecov.io.

Directory.Build.props

Tanya-Solyanik

Thank you! Make sure to squash-merge this PR!

Tanya-Solyanik · 2024-01-09T19:43:22Z

That is the expectation. It's not clear yet why it's not showing up in this repository, but you can see it in action here:
dotnet/roslyn-analyzers#7116

I'll double-check the differences in these two pipelines to enable the feature in codecov.io.

Feel free to do that in the next PR.

Tanya-Solyanik · 2024-01-10T04:24:51Z

codecov.yml

@@ -20,16 +20,29 @@ flags:                            # which files to include in the reporting
  production:
    paths:
      - src/Common/src/
+      - src/Microsoft.VisualBasic/src/
+      - src/Microsoft.VisualBasic.Forms/src/
      - src/System.Design/src/
      - src/System.Drawing/src/


We don't have to include System.Design and Drawing into reporting, do we still need it here?

The changes here are mainly for Codecov.io, if, however, they are not needed, I will remove them in the PR adapted to Codecov.io.

ghost assigned SimonZhao888 Dec 14, 2023

ghost added the draft draft PR label Dec 14, 2023

RussKie reviewed Dec 20, 2023

View reviewed changes

eng/MergeCoverageData.ps1 Outdated Show resolved Hide resolved

RussKie reviewed Dec 22, 2023

View reviewed changes

src/Microsoft.VisualBasic.Forms/tests/UnitTests/Microsoft.VisualBasic.Forms.Tests.vbproj Outdated Show resolved Hide resolved

RussKie reviewed Dec 22, 2023

View reviewed changes

eng/pipelines/build.yml Outdated Show resolved Hide resolved

SimonZhao888 commented Dec 27, 2023

View reviewed changes

eng/pipelines/build.yml Outdated Show resolved Hide resolved

SimonZhao888 requested a review from RussKie December 28, 2023 06:01

SimonZhao888 force-pushed the dev/v-weidzh/UnitTestCoverage branch from 8a55641 to fd178b0 Compare December 28, 2023 06:44

SimonZhao888 requested review from Tanya-Solyanik, LeafShi1, Epica3055 and sharwell December 28, 2023 06:47

SimonZhao888 removed the draft draft PR label Dec 28, 2023

SimonZhao888 marked this pull request as ready for review December 28, 2023 06:48

SimonZhao888 requested a review from a team as a code owner December 28, 2023 06:49

SimonZhao888 requested a review from ricardobossan December 28, 2023 07:24

LeafShi1 reviewed Dec 28, 2023

View reviewed changes

eng/MergeCoverageData.ps1 Outdated Show resolved Hide resolved

eng/MergeCoverageData.ps1 Outdated Show resolved Hide resolved

SimonZhao888 requested a review from LeafShi1 December 28, 2023 08:28

Tanya-Solyanik added the waiting-author-feedback The team requires more information from the author label Dec 31, 2023

ghost removed the waiting-author-feedback The team requires more information from the author label Jan 2, 2024

Tanya-Solyanik reviewed Jan 2, 2024

View reviewed changes

Tanya-Solyanik reviewed Jan 8, 2024

View reviewed changes

eng/CodeCoverage.proj Outdated Show resolved Hide resolved

Tanya-Solyanik reviewed Jan 8, 2024

View reviewed changes

eng/pipelines/build.yml Outdated Show resolved Hide resolved

Tanya-Solyanik reviewed Jan 8, 2024

View reviewed changes

...mitives/tests/TestUtilities.Tests/System.Windows.Forms.Primitives.TestUtilities.Tests.csproj Outdated Show resolved Hide resolved

Simon Zhao added 4 commits January 9, 2024 10:12

Merge branch 'main' into dev/v-weidzh/UnitTestCoverage

79e71d1

Revert useless changes

944123e

Revert useless changes

147d8c1

Revert useless changes

86af83f

Tanya-Solyanik reviewed Jan 9, 2024

View reviewed changes

Directory.Build.props Outdated Show resolved Hide resolved

Add the projects to Codecov

70b13d8

Tanya-Solyanik previously approved these changes Jan 9, 2024

View reviewed changes

JeremyKuhne previously approved these changes Jan 9, 2024

View reviewed changes

lonitra previously approved these changes Jan 9, 2024

View reviewed changes

Adjust the includ projects to code coverage

f6b8649

SimonZhao888 dismissed stale reviews from lonitra, JeremyKuhne, and Tanya-Solyanik via f6b8649 January 10, 2024 01:32

Simon Zhao added 3 commits January 10, 2024 09:47

Adjust the includ projects to code coverage

1863f4a

Adjust the includ projects to code coverage

935334f

Adjust the includ projects to code coverage

5e38f6a

SimonZhao888 requested a review from Tanya-Solyanik January 10, 2024 03:05

Tanya-Solyanik reviewed Jan 10, 2024

View reviewed changes

Tanya-Solyanik approved these changes Jan 10, 2024

View reviewed changes

SimonZhao888 merged commit 41e805c into dotnet:main Jan 10, 2024
9 checks passed

ghost added this to the 9.0 Preview1 milestone Jan 10, 2024

SimonZhao888 mentioned this pull request Jan 17, 2024

Remove System.Design and Drawing from codecov.yml file #10678

Merged

github-actions bot locked and limited conversation to collaborators Feb 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing all WinForms controls have unit tests coverage #10476

Testing all WinForms controls have unit tests coverage #10476

SimonZhao888 commented Dec 14, 2023 •

edited by Tanya-Solyanik

Loading

Tanya-Solyanik commented Dec 15, 2023

RussKie commented Dec 17, 2023 •

edited

Loading

sharwell commented Dec 18, 2023

RussKie commented Dec 19, 2023

sharwell commented Dec 19, 2023 •

edited

Loading

sharwell commented Dec 28, 2023 •

edited

Loading

SimonZhao888 commented Jan 2, 2024

Tanya-Solyanik commented Jan 2, 2024

sharwell commented Jan 2, 2024

Tanya-Solyanik Jan 2, 2024

SimonZhao888 Jan 3, 2024

sharwell Jan 4, 2024 •

edited

Loading

SimonZhao888 Jan 5, 2024

sharwell Jan 5, 2024

sharwell Jan 5, 2024 •

edited

Loading

Tanya-Solyanik commented Jan 8, 2024

sharwell commented Jan 8, 2024

SimonZhao888 commented Jan 9, 2024

Tanya-Solyanik left a comment

Tanya-Solyanik commented Jan 9, 2024

Tanya-Solyanik Jan 10, 2024 •

edited

Loading

SimonZhao888 Jan 10, 2024

Testing all WinForms controls have unit tests coverage #10476

Testing all WinForms controls have unit tests coverage #10476

Conversation

SimonZhao888 commented Dec 14, 2023 • edited by Tanya-Solyanik Loading

Proposed changes

Customer Impact

Regression?

Risk

Screenshots

Test methodology

Microsoft Reviewers: Open in CodeFlow

Tanya-Solyanik commented Dec 15, 2023

RussKie commented Dec 17, 2023 • edited Loading

sharwell commented Dec 18, 2023

RussKie commented Dec 19, 2023

sharwell commented Dec 19, 2023 • edited Loading

sharwell commented Dec 28, 2023 • edited Loading

SimonZhao888 commented Jan 2, 2024

Tanya-Solyanik commented Jan 2, 2024

sharwell commented Jan 2, 2024

Tanya-Solyanik Jan 2, 2024

Choose a reason for hiding this comment

SimonZhao888 Jan 3, 2024

Choose a reason for hiding this comment

sharwell Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

SimonZhao888 Jan 5, 2024

Choose a reason for hiding this comment

sharwell Jan 5, 2024

Choose a reason for hiding this comment

sharwell Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Tanya-Solyanik commented Jan 8, 2024

sharwell commented Jan 8, 2024

SimonZhao888 commented Jan 9, 2024

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Tanya-Solyanik commented Jan 9, 2024

Tanya-Solyanik Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

SimonZhao888 Jan 10, 2024

Choose a reason for hiding this comment

SimonZhao888 commented Dec 14, 2023 •

edited by Tanya-Solyanik

Loading

RussKie commented Dec 17, 2023 •

edited

Loading

sharwell commented Dec 19, 2023 •

edited

Loading

sharwell commented Dec 28, 2023 •

edited

Loading

sharwell Jan 4, 2024 •

edited

Loading

sharwell Jan 5, 2024 •

edited

Loading

Tanya-Solyanik Jan 10, 2024 •

edited

Loading