winforms icon indicating copy to clipboard operation
winforms copied to clipboard

Code Coverage for DownloadFile Review 1St

Open paul1956 opened this issue 1 year ago • 9 comments

Replace corrupt PR12221

Proposed changes

  • Code Coverage for DownloadFile

Customer Impact

  • This adds code coverage for DownloadFile which will be required to replace obsolete WebClient.

Regression?

  • No

Risk

-None

Test environment(s)

Microsoft Reviewers: Open in CodeFlow

paul1956 avatar Nov 14 '24 19:11 paul1956

Codecov Report

:x: Patch coverage is 97.56175% with 77 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 77.34276%. Comparing base (6d20991) to head (6e9ca2b). :warning: Report is 1 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #12486         +/-   ##
===================================================
+ Coverage   77.15155%   77.34276%   +0.19120%     
===================================================
  Files           3278        3285          +7     
  Lines         645199      648349       +3150     
  Branches       47716       47895        +179     
===================================================
+ Hits          497781      501451       +3670     
+ Misses        143724      143200        -524     
- Partials        3694        3698          +4     
Flag Coverage Δ
Debug 77.34276% <97.56175%> (+0.19120%) :arrow_up:
integration 18.98929% <ø> (-0.00036%) :arrow_down:
production 52.22702% <ø> (+0.20664%) :arrow_up:
test 97.40849% <97.56175%> (+0.00185%) :arrow_up:
unit 49.66504% <ø> (+0.20142%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Nov 14 '24 19:11 codecov[bot]

I would suggest factoring out the following files into a standalone PR: SingleInstanceHelpers.vb WindowsFormsApplicationBase.vb Network.DownloadFile.vb Interaction.vb ClipboardProxy.vb SingleInstanceHelpersTests.vb TestUtilitiesTests.vb PathSeparatorTestData.vb VbFileCleanupTestBase.vb

They contain only cleanup changes and are ready to be merged while we discuss HTTPListener use within the team.

@Tanya-Solyanik #12730 is all the cleanup

paul1956 avatar Jan 08 '25 08:01 paul1956

@Tanya-Solyanik @KlausLoeffelmann I updated the logic to have a text (json) file that contains all the server information. If someone had a real server, they could update the text file and use a local (or public) server to do testing. This also allows me to test with servers I have access to without modifying any code, I am finding many issues with tests that I am fixing. I am currently testing all the changes with multiple servers; all the tests pass with WebListener on Windows 11 and they pass on GitHub. There is probable a way to use an internal server and not expose username and password in the repo and remove WebListener.

Of note: the file location is odd compared to where the other tests find json files.

Path.Combine(My.Application.Info.DirectoryPath, "System\Windows\TestUtilities\TestData", "ServerConfiguration.JSON")
Path.Combine(My.Application.Info.DirectoryPath, "ServerConfiguration.JSON")

Visual Studio is picking the location, so I don't know how to fix.

paul1956 avatar Jan 14 '25 02:01 paul1956

Is this meant as the "security net" for the current API, so should we switch, we know it did not regress?

KlausLoeffelmann avatar Oct 25 '25 04:10 KlausLoeffelmann

@LeafShi1 - anybody of you able to do a review? I know it's VB. But, I can't currently spend much time with this.

@Copilot: Could you create an evaluation list of issues we should take a closer look on? Create a Markdown Todo list in a comment, which could serve as the template for an issue. Be very thorough, but not nit-picky.

KlausLoeffelmann avatar Oct 25 '25 04:10 KlausLoeffelmann

Sort of. I first run the tests against public upload and download servers locally to verify that the tests are correct, then implemented my own server in this PR and all the tests need to pass since I was told I can’t test against real public servers on GitHub. Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Oct 25, 2025, at 12:11 AM, Klaus Löffelmann @.***> wrote:KlausLoeffelmann left a comment (dotnet/winforms#12486) Is this meant as the "security net" for the current API, so should we switch, we know it did not regress?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: @.***>

paul1956 avatar Oct 25 '25 04:10 paul1956

Forgot I pulled upload changes because there were too many issues and the PR became too big and it’s probably not needed. Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Oct 25, 2025, at 12:26 AM, Roslyn.Cohen @.> wrote:Sort of. I first run the tests against public upload and download servers locally to verify that the tests are correct, then implemented my own server in this PR and all the tests need to pass since I was told I can’t test against real public servers on GitHub. Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Oct 25, 2025, at 12:11 AM, Klaus Löffelmann @.> wrote:KlausLoeffelmann left a comment (dotnet/winforms#12486) Is this meant as the "security net" for the current API, so should we switch, we know it did not regress?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: @.***>

paul1956 avatar Oct 25 '25 04:10 paul1956

I am wondering, if it makes sense, to now, as the mechanical maintenance has already been done in a separate PR, to create a new one, with just the changes relevant to what's left for the actual unit tests.

The Commits would pretty much polute the history.

Are the changes for the actual tests pretty central?

KlausLoeffelmann avatar Oct 25 '25 04:10 KlausLoeffelmann

I assume someone could either squash the commits or create a new Pr with the few files. They are all new in this PR.Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Oct 25, 2025, at 12:34 AM, Klaus Löffelmann @.***> wrote:KlausLoeffelmann left a comment (dotnet/winforms#12486) I am wondering, if it makes sense, to now, as the mechanical maintenance has already been done in a separate PR, to create a new one, with just the changes relevant to what's left for the actual unit tests. The Commits would pretty much polute the history. Are the changes for the actual tests pretty central?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were assigned.Message ID: @.***>

paul1956 avatar Oct 25 '25 05:10 paul1956