Code Coverage for DownloadFile Review 1St #12486
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12486 +/- ##
===================================================
+ Coverage 76.14673% 76.34958% +0.20284%
===================================================
Files 3191 3195 +4
Lines 640105 642830 +2725
Branches 47233 47372 +139
===================================================
+ Hits 487419 490798 +3379
+ Misses 149166 148510 -656
- Partials 3520 3522 +2
Flags with carried forward coverage won't be shown. Click here to find out more. |
Cleanup
Move test constants to separate module for future reuse
Move some constants around Rename some files that are shared between upload and download
Fix Upload logic to handle Web error 500
Correct a test
There was a problem hiding this comment.
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.
src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network.UploadFile.vb
Outdated
Show resolved
Hide resolved
| ''' </summary> | ||
| ''' <returns><see langword="True"/> if an audio stream is available, otherwise <see langword="False"/>.</returns> | ||
| ''' <returns> | ||
| ''' see langword="True"/> if an audio <see cref="Stream"/> is available, |
There was a problem hiding this comment.
This could be a single line, it's not long enough to be broken.
|
|
||
| Friend Function ProcessRequests() As HttpListener | ||
| ' Create a listener and add the prefixes. | ||
| Dim listener As New HttpListener() |
There was a problem hiding this comment.
For my education, what is the purpose of WebClientCopy class?
src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/NetworkUtilities.vb
Outdated
Show resolved
Hide resolved
| _webClient.DownloadFile(address, destinationFileName) | ||
| End If | ||
| Catch ex As WebException | ||
| If ex.Message.Contains("401") Then |
There was a problem hiding this comment.
It's usually good to keep the original exception for context. Why do we have to introduce our own messages? Can we set the original exception as the inner one?
There was a problem hiding this comment.
THe same comment applies to the Upload method.
There was a problem hiding this comment.
My WebServerListner is implemented with HTTPClient, the existing code expects WebClient Errors. This code translates the errors, so they are compatible with existing code. When WebClient and WebClientCopy go away and upload and download use HTTPClient the same translation will happen.
There was a problem hiding this comment.
WinForms only handle 2 specific Exceptions Timeout, Unauthorized plus a generic WebException.
When I run the existing code using WebClient I can get almost complete coverage of DownloadFile and UploadFile and the generic exception is never used. With HttpClient I handle the generic exception and use it for testing.
There was a problem hiding this comment.
Hm, we shouldn't add new exceptions for testing purposes only. New exceptions is a breaking change and we need an apparent benefit gained from making such a change.
There was a problem hiding this comment.
It's usually good to keep the original exception for context. Why do we have to introduce our own messages? Can we set the original exception as the inner one?
@Tanya-Solyanik
We are reproducing WebClient Exceptions, HttpClient returns HttpErrors. From what I can tell WinForms only cares about 3 WebClient Exceptions (Timeout, Unauthorized and a generic catch all which is not used by WinForms today).
I have no visibility into all the errors that WebClient can possibly throw but WinForms works consistently with know and unknown errors.
The goal is no breaking exception changes.
src/Microsoft.VisualBasic.Forms/tests/UnitTests/System/Windows/Forms/FileSystemProxyTests.vb
Outdated
Show resolved
Hide resolved
Tanya-Solyanik
added
the
📭 waiting-author-feedback
dotnet-policy-service
bot
removed
the
📭 waiting-author-feedback
@Tanya-Solyanik #12730 is all the cleanup |
Tanya-Solyanik
added
the
📭 waiting-author-feedback
dotnet-policy-service
bot
removed
the
📭 waiting-author-feedback
Replace corrupt PR12221
Proposed changes
Customer Impact
Regression?
Risk
-None
Test environment(s)
Microsoft Reviewers: Open in CodeFlow