Code Coverage for DownloadFile Review 1St
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
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.
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
@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.
Is this meant as the "security net" for the current API, so should we switch, we know it did not regress?
@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.
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: @.***>
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: @.***>
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?
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: @.***>