Fix issue #9807 Review AFTER PR ##11655

[paul1956]

Fixes #9807

Proposed changes

• Add Remove dependance on WebClient for VB FileDownload by using HttpClient
• Add a Visual Basic Scratch Project so that Microsoft.VisualBasic.Forms dependent code can be tested from a Visual Basic project (even an empty WinForms project written in VB does not work)
• Upload still uses WebClient that also needs to be done as a separate PR

Customer Impact

• Adds Async file download support to VB, and if WebClient goes away this will allow existing Visual Basic programs using synchronous File Download to still work.

Regression?

• Yes when WebClient if and when goes away, no today

Risk

• The code as is completely untested as I can't test without a working Scratch Project.
• The existing code uses ICredentials in some of the paths, it is unclear how compatible that is between WebClient and HttpClient

Test methodology

• EXISTING CODE IS UNTESTED, and don't know if any tests currently exist for FileDownload.
• I will write tests if none exist.

Test environment(s)

• TBD

• Visual Basic code to replace deprecated WebClient, before it is removed from .Net

Microsoft Reviewers: Open in CodeFlow

[paul1956]

@elachlan I have removed the Scratch Project and move all the tests into a new Network tests file to unit/Microsoft.VisualBasic.Forms.Tests/VisualBasic.Tests. under Forms it turns out there were no tests for upload or download just a comment.

I commented out all the test code because Test Projects don't have access to My.Computer.Network. All the tests would pass except for the one using the UI and the errors returned are the same as the current functions. The UI tests fail in a real VB project. I also made DownloadFileAsync Private, so this adds no new API services. There is nothing else I can do without help at this time. To make DownloadFileAsync Public the ProgressBar would need to be addressed.

[KlausLoeffelmann]

I commented out all the test code because Test Projects don't have access to My.Computer.Network...

Why is that?
I am just on my phone right now, but I'd say, there should be a way to check-in the additional generated classes which get created on writing the application.myapp file?

[paul1956]

I commented out all the test code because Test Projects don't have access to My.Computer.Network...

Why is that? I am just on my phone right now, but I'd say, there should be a way to check-in the additional generated classes which get created on writing the application.myapp file?

@KlausLoeffelmann Sorry that is not enough information for me to know how to fix this issue.
My in tests is not the same as My in VB WinForms projects.

[KlausLoeffelmann]

That sounds to me that the tests don't reflect the settings for the Application Framework enabled.

Sorry that is not enough information for me to know how to fix this issue...

Have you guys read this (especially the paragraphed I tagged:)?

https://devblogs.microsoft.com/dotnet/update-to-winforms-vb-appframework/#a-look-behind-the-scenes-of-the-winforms-vb-application-framework

It's quite probable that it'll help in the context.
I don't have the time to do it right now, but we should find a way to include the Application Framework generated files in the unit tests. So, again, without really having dig into the whole topic, maybe checking in the code generated part helps here.

[paul1956]

@KlausLoeffelmann Figured something out, adding the 3 lines below make the tests compile.

<MyType>WindowsFormsWithCustomSubMain</MyType>
<UseApplicationFramework>True</UseApplicationFramework>
<OutputType>Library</OutputType>

All Tests pass in local computer and all pass on CI with 1 skipped.
Exceptions are identical to original code.

[KlausLoeffelmann]

Nice!

It'll take a bit, until I am able to dig deeper into it, but I have it on my radar.
We won't take this for .NET 8, since it's to late, but I think it's also not that urgent, so .NET 9 is fine. Please remember, we also need the sync version for backwards compat and the exact same overloads.

And since we're introducing async versions, we need a list of the new APIs and a rationale. The rationale needs to point out, that the primary motivation is not to introduce async overloads, but replacing an obsolete usage of an internal API - because this needs to go through the API Review Board.

And please don't rush it, it's taking some time to get this in, anyway!

[paul1956]

@KlausLoeffelmann The Async Overloads are private. I added no new public functions (there is 1 new SR message because I didn't know how to access the existing one). Making them public would require more work as ProgressDialog is also not Public. The implementation of the existing API's is Async they call the private DownloadFileAsync functions, there is just an Await at the end.

I also don't see a way to sign the license.

[elachlan]

Paul, The license thing is a CI bug I think.

[elachlan]

Some tests are failing. Very close though.

[paul1956]

Some tests are failing. Very close though.

@elachlan
No tests are failing locally, how can I find out what is failing? I don't have access to D:

It is failing a Release build but there are no #IF Not Debug code paths in the code I changed.

It seems to be failing on .Net 9 which I don't have.

This really should wait until VB-Code-Cleanup PR to avoid duplicate work.

[paul1956]

@elachlan I have changed nothing except merging with main and now the tests passed.

[elachlan]

Probably a transient test issue. It happens.

[Tanya-Solyanik]

Assert.Throws() Failure: No exception was thrown\r\nExpected: typeof(System.Net.WebException)

Stack trace
at Microsoft.VisualBasic.Forms.Tests.Microsoft.VisualBasic.Forms.Tests.NetworkTexts.SimpleFileDownloadWithExpectedTimeOut_method() in /_/src/Microsoft.VisualBasic.Forms/tests/UnitTests/System/Windows/Forms/NetworkTexts.vb:line 100
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InterpretedInvoke_Method(Object obj, IntPtr* args)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[Tanya-Solyanik]

Why did you repeat 4 rules that are already listed under C#+VB section?

[paul1956]

Because C# and VB need different options, so I deleted them from common section and preserved C# values and changed VB values. To do this you need to duplicate the sections under C# and #VB so these 4 times appear twice with different options. Lines 204-207 are the VB Versions

[KlausLoeffelmann]

Out of interest more than anything else (since I am pretty sure you know what you're doing here! 😸):
What exact effects do these changes have? I remember you talked about auto-adding empty lines on save/through the pretty lister at one point, if I remember correctly?

[paul1956]

Ping, is there an ETA when someone might review this?

[KlausLoeffelmann]

No, not immediately. I just finalized a big VB change for CodeGen in the Designer, so VB won't get another higher prio tasks this or next week.

@paul1956: What exactly do you mean by this?

[paul1956]

@KlausLoeffelmann old comment. I wrote tests for all the non interactive code, there are no tests specific to the interactive code though I have tested the interactive code, in a separate project on my machine. To test the interactive code inside this solution I would need a VB scratch project like exists for the C# code and creating one is not obvious.

[codecov]

Codecov Report

Attention: Patch coverage is 77.24758% with 987 lines in your changes missing coverage. Please review.

Project coverage is 75.07239%. Comparing base (f53f153) to head (04e01da).
Report is 301 commits behind head on feature/10.0.

Additional details and impacted files

@@                  Coverage Diff                   @@
##           feature/10.0       #9867         +/-   ##
======================================================
+ Coverage      74.29395%   75.07239%   +0.77844%     
======================================================
  Files              3026        3067         +41     
  Lines            627152      633021       +5869     
  Branches          46758       46853         +95     
======================================================
+ Hits             465936      475224       +9288     
+ Misses           157863      154437       -3426     
- Partials           3353        3360          +7     

Flag	Coverage Δ
Debug	75.07239% <77.24758%> (+0.77844%)	⬆️
integration	17.80547% <7.35706%> (-0.18468%)	⬇️
production	48.21708% <60.89564%> (+1.18681%)	⬆️
test	97.02284% <99.51007%> (+0.03516%)	⬆️
unit	45.33407% <54.69812%> (+1.32249%)	⬆️

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

[paul1956]

@KlausLoeffelmann an update on testing, functions that require passwords were not tested by me or any of the tests I added. I would need a server that supports username and password that I could download the test files from and a way to do it without exposing the server to attack. Also, interactive functions were tested using separate project. I don't know how to write interactive tests. None of this if different than what was here before the file Network Tests is new. I have not exposed any new API's, DownloadFileAsync is private and could be made public but that requires API review and exposing ProgressDialog and a decision on how many of the options to make public.

[elachlan]

@Olina-Zhang and team might be able to help test those.

I'd say we should land it as private and do an API Review afterwards to move it to public.

[paul1956]

Replaces #11867, all PR Feedback is addressed and now targets feature/10.0