-
Notifications
You must be signed in to change notification settings - Fork 998
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Code Coverage for DownloadFile Review 1St #12486
base: main
Are you sure you want to change the base?
Changes from all commits
1cd54bf
bf912fe
1adf2be
af840f2
8f280c4
d1b4234
4e48605
1782915
10c1989
2d92096
4324b8b
ebdc88d
2718370
175440a
f1c6e65
fa37a7b
fbd2c7c
0baeb02
8977f10
7f51bcc
a9eb2f5
5b89c04
2b7303c
ffe7e35
db6bdf6
ccdf42b
7324bb1
65cbc86
f3cc2d0
cdab3aa
47f12cd
e3ce2e2
6a82523
2110608
a3ead0c
41f8a19
73aa2cd
eb6ba5c
a7b20cc
c633809
4f9d051
354ccdf
468e669
6e313e2
b8de54b
70283e4
0fa9cd3
a6413df
8449f48
6e3b58e
d3e5523
ad1bab1
c67f9cf
758772a
ac82c9f
e2ff073
5f956f4
0b70c5d
6f59c93
a36b059
6509c30
395de61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THe same comment applies to the Upload method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.