Code Coverage for DownloadFile Review 1St - #12486
Open
paul1956 wants to merge 125 commits into
Open
Conversation
Codecov Report❌ Patch coverage is 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
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
3 tasks
Cleanup
Move test constants to separate module for future reuse
Move some constants around Rename some files that are shared between upload and download
Member
|
Is this meant as the "security net" for the current API, so should we switch, we know it did not regress? |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for the DownloadFile functionality in Microsoft.VisualBasic.Forms, which will eventually replace the obsolete WebClient. The changes include new test infrastructure, extensive test cases for both download and upload scenarios, and supporting utility classes for server configuration and test data management.
Key Changes
- Added complete test suite for
DownloadFilefunctionality with 1,722 lines of tests covering various scenarios including authentication, timeout, and file handling - Created supporting test infrastructure including
WebListener,ServerConfiguration, and helper utilities for managing test files and verifying downloads - Added test coverage for
UploadFilefunctionality with 1,472 lines of tests - Introduced new localized resource strings for web status errors (NotFound, Timeout, Unauthorized)
Reviewed Changes
Copilot reviewed 28 out of 29 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| DownloadFileTests.vb | Comprehensive test suite for DownloadFile functionality covering URI/URL variants, authentication, timeout, overwrite scenarios |
| UploadFileTests.vb | Complete test coverage for UploadFile with authentication, error handling, and various parameter combinations |
| WebListener.vb | Test utility class that implements HTTP listener for simulating download/upload server operations |
| ServerConfiguration.vb | Configuration management for test server settings with JSON serialization support |
| DownloadFileVerifiers.vb | Helper module providing verification methods for successful and failed download operations |
| DownloadFileTestConstants.vb | Centralized test constants for URLs, passwords, and timeout values |
| EnumFileSizes.vb | Enum defining standard file sizes used across tests |
| PrivateSetterContractResolver.vb | JSON serialization helper for handling properties with private setters |
| VbFileCleanupTestBase.vb | Updated CreateTempFile to use FileSizes enum and improved documentation formatting |
| FileSystemProxyTests.vb | Updated to use FileSizes enum instead of integer literals |
| ServerConfigurationTests.vb | Tests for server configuration serialization and deserialization |
| SR.resx and xlf files | Added new localized resource strings for web status error messages |
| Winforms.sln | Removed DemoConsole project reference |
|
|
||
| ''' <summary> | ||
| ''' Parses a <see cref="HttpListenerRequest"/> and gets the fileName of the uploaded file | ||
| ''' and the lenght of the data file in bytes |
There was a problem hiding this comment.
Corrected spelling of 'lenght' to 'length'.
Suggested change
| ''' and the lenght of the data file in bytes | |
| ''' and the length of the data file in bytes |
Comment on lines
+265
to
+269
| ''' and the lenght of the data file in bytes | ||
| ''' </summary> | ||
| ''' <param name="request"></param> | ||
| ''' <returns> | ||
| ''' A <see cref="Dictionary(Of String, String)"/> that contains the filename and lenght of the data file. |
There was a problem hiding this comment.
Corrected spelling of 'lenght' to 'length'.
Suggested change
| ''' and the lenght of the data file in bytes | |
| ''' </summary> | |
| ''' <param name="request"></param> | |
| ''' <returns> | |
| ''' A <see cref="Dictionary(Of String, String)"/> that contains the filename and lenght of the data file. | |
| ''' and the length of the data file in bytes | |
| ''' </summary> | |
| ''' <param name="request"></param> | |
| ''' <returns> | |
| ''' A <see cref="Dictionary(Of String, String)"/> that contains the filename and length of the data file. |
| End If | ||
| Dim filenameWithPath As String = Path.Join(sourceDirectoryName, filename) | ||
|
|
||
| If size >= 0 Then |
There was a problem hiding this comment.
The condition size >= 0 is inconsistent with the updated enum type FileSizes. The comparison should check against FileSizes.Unknown instead of comparing to an integer literal, since size is now of type FileSizes.
Suggested change
| If size >= 0 Then | |
| If size <> FileSizes.Unknown Then |
KlausLoeffelmann
added
🚧 work in progress
Member
|
@LeafShi1 - anybody of you able to do a review? @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. |
Contributor
Author
|
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: ***@***.***>
|
Contributor
Author
|
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: ***@***.***>
|
Member
|
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? |
Contributor
Author
|
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: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replace corrupt PR12221
Proposed changes
Customer Impact
Regression?
Risk
-None
Test environment(s)
Microsoft Reviewers: Open in CodeFlow