Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

paul1956 · 2024-09-11T08:49:52Z

Fixes VB Long lines

Proposed changes

shorten long lines in VB

Customer Impact

None

Regression?

No

Risk

-Minimal No code changes except formatting

Microsoft Reviewers: Open in CodeFlow

codecov · 2024-09-11T09:23:54Z

Codecov Report

Attention: Patch coverage is 90.89128% with 372 lines in your changes missing coverage. Please review.

Project coverage is 76.38918%. Comparing base (776484e) to head (061cb10).

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #12118         +/-   ##
===================================================
+ Coverage   76.18249%   76.38918%   +0.20668%     
===================================================
  Files           3194        3202          +8     
  Lines         640365      644086       +3721     
  Branches       47239       47525        +286     
===================================================
+ Hits          487846      492012       +4166     
+ Misses        148983      148539        -444     
+ Partials        3536        3535          -1

Flag	Coverage Δ
Debug	`76.38918% <90.89128%> (+0.20668%)`	⬆️
integration	`18.13305% <6.26223%> (-0.03019%)`	⬇️
production	`50.42868% <75.14677%> (+0.31110%)`	⬆️
test	`97.01678% <96.14631%> (-0.00319%)`	⬇️
unit	`47.83347% <69.47162%> (+0.29377%)`	⬆️

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

...otnet.WinForms.ProjectTemplates/content/WinFormsApplication-VisualBasic/ApplicationEvents.vb

...Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkAvailability.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkDownload.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkUpload.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/ClipboardProxy.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/FileSystemProxy.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/Internal/HttpClientCopy.vb

src/Microsoft.VisualBasic.Forms/tests/UnitTests/System/Windows/Forms/NetworkDownloadTests.vb

Tanya-Solyanik

@paul1956 - is this PR ready for a review, or are you still waiting to merge previous PRs? I would suggest factoring out parts of this PR that are not based on the previous changes and are dedicated only to comments and long code lines.

...ft.VisualBasic.Forms/src/Microsoft/VisualBasic/ApplicationServices/ConsoleApplicationBase.vb

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Clock.vb

Tanya-Solyanik · 2024-09-13T17:30:09Z

...Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Devices/Network/NetworkAvailability.vb

+
+Imports NetInfoAlias = System.Net.NetworkInformation
+
+Namespace Microsoft.VisualBasic.Devices


Why did you move this file, the namespace matches Microsoft\VisualBasic\Devices path. And this directory contains only 11 items, so it is manageable. I don't see a reason for this move.

Previous Feedback to move classes into their own file

Into their own file, not into a new folder.

@Tanya-Solyanik The entire focus of all the VB PR's was to fix the networking code. Putting it all together in a directory just make it simpler for me. Originally it was all in 1 file. I have moved them back.

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/Helpers/SafeNativeMethods.vb

Tanya-Solyanik · 2024-09-13T17:39:17Z

src/Microsoft.VisualBasic.Forms/src/Microsoft/VisualBasic/MyServices/Internal/HttpClientCopy.vb

+Imports System.Threading
+Imports Microsoft.VisualBasic.Devices.NetworkUtilities
+
+Namespace Microsoft.VisualBasic.MyServices.Internal


Could you please pull out this refactoring from this PR into a separate one? It will be easier to review.

@Tanya-Solyanik It is, this PR should be reviewed in order then only 6 files core to the issue are changed, httpClientCopy is the whole purpose of this PR. I have changed everything to DRAFT except 1.

Sorry my lack of knowledge of PR process has caused you to do large reviews and almost null reviews.

Everything I have done is to get to this PR which has the critical fix as webClient is going to be made obsolete.

Did you factor out "critical fix as webClient"?

Did you factor out "critical fix as webClient"?

@Tanya-Solyanik the Critical fix was to replace WebClient with HttpClient for both network upload and download.

WebCient was split into two parts the Upload part is currently unchanged because I have no acceptable way to test without 3rd party code/server (my prototype used public servers to test existing code). I created my own download server in the repo so I can test download and replaced WebClient with HttpClient for download only.

We shouldn't test WebClient or HttpClient because we don't own them.

@Tanya-Solyanik the replacement needs to return the identical error codes to not be a breaking change. I am not testing WebClient or HttpClient by themselves just as they are used in NetworkDownload.

Tanya-Solyanik · 2024-09-13T17:40:27Z

src/Microsoft.VisualBasic.Forms/src/Properties/AssemblyInfo.vb

+
+Imports System.Runtime.CompilerServices
+
+<Assembly: InternalsVisibleTo("Microsoft.VisualBasic.Forms.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")>


Is this coming from another PR?

Yes one being discussed by @LeafShi1 and @KlausLoeffelmann

ricardobossan · 2024-09-16T10:13:17Z

src/System.Windows.Forms/src/System/Windows/Forms/Input/InputLanguage.cs

@@ -109,7 +109,8 @@ public string LayoutName
    }

    /// <summary>
-    ///  Returns the <see href="https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getkeyboardlayoutnamew">
+    ///  Returns the
+    ///  <see href="https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getkeyboardlayoutnamew">
    ///  keyboard layout identifier</see> of the current input language.


I think this better follows the pattern of the other see tags in the source code:

/// <summary> /// Returns the /// <see href="https://learn.microsoft.com/windows/win32/api/winuser/nf-winuser-getkeyboardlayoutnamew"> /// keyboard layout identifier /// </see> /// of the current input language. /// </summary>

@ricardobossan good feedback but why are you reviewing a draft PR? All the draft PRs depend on the previous ones. So there would be a tremendous amounts of duplicate work. There are 3 that need to merge before this one.

Because there were comments being made already. But I'll wait out next time.

@ricardobossan sorry that was my fault I did not understand the review process and only learned about Draft Friday.

There are several more I did without Draft that could be reviewed but I will need a little time at apply this comment to them.

Thanks for your understanding.

@ricardobossan opened a new PR #12151 based on main to just deal with this issue, there are just a few places where it does not follow the pattern. I will address expected merge conflicts in future PR's where they occur.

…07-2nd-DRAFT

The server will now write issues as text to a public property ServerFault. The tests can then verify that there were no protocal issues except here expected.

…07-2nd-DRAFT

…Review-after-FixIssue#9807-3rd

Fix error in Progress Progress Dialog Labels Use Enum to define testing file sizes Get FileSize and URL from WebListner to allow 1 source of truth This will also allow easier testing with real file server

…07-2nd-DRAFT

Allow skipping of tests that need server to support Unauthorized Access based on user reedentials. Make logic for file Upload detection not dependant on file name to support other Servers in testing.

Add support for real File Server Testing

…e servers

…ceptions in existing code.

Improve code coverage for upload tests Support more real File Server options

…07-2nd-DRAFT

paul1956 requested a review from a team as a code owner September 11, 2024 08:49

dotnet-policy-service bot assigned paul1956 Sep 11, 2024

Tanya-Solyanik requested review from LeafShi1, SimonZhao888, Epica3055 and ricardobossan September 11, 2024 21:30

Tanya-Solyanik added waiting-review This item is waiting on review by one or more members of team 📭 waiting-author-feedback The team requires more information from the author and removed waiting-review This item is waiting on review by one or more members of team labels Sep 11, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 11, 2024

paul1956 changed the title ~~Shorten long lines in VB review after fix issue#9807~~ Shorten long lines in VB REVIEW AFTER fix issue#9807 Sep 12, 2024

LeafShi1 reviewed Sep 12, 2024

View reviewed changes

LeafShi1 added 📭 waiting-author-feedback The team requires more information from the author a11y-external An accessibility bug that is external to WinForms (for example, a Common Dialog in Win32) labels Sep 12, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 12, 2024

Tanya-Solyanik added 📭 waiting-author-feedback The team requires more information from the author and removed a11y-external An accessibility bug that is external to WinForms (for example, a Common Dialog in Win32) labels Sep 12, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 13, 2024

Tanya-Solyanik reviewed Sep 13, 2024

View reviewed changes

Tanya-Solyanik added the 📭 waiting-author-feedback The team requires more information from the author label Sep 13, 2024

dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Sep 13, 2024

paul1956 changed the title ~~Shorten long lines in VB REVIEW AFTER fix issue#9807~~ Shorten long lines in VB REVIEW AFTER fix issue#9807 DRAFT Sep 13, 2024

Tanya-Solyanik added the draft draft PR label Sep 14, 2024

Tanya-Solyanik marked this pull request as draft September 14, 2024 00:23

paul1956 changed the title ~~Shorten long lines in VB REVIEW AFTER fix issue#9807 DRAFT~~ Shorten long lines in VB REVIEW (4th) AFTER fix issue#9807 DRAFT Sep 16, 2024

ricardobossan reviewed Sep 16, 2024

View reviewed changes

paul1956 changed the title ~~Shorten long lines in VB REVIEW (4th) AFTER fix issue#9807 DRAFT~~ Shorten long lines in VB REVIEW (3rd) AFTER fix issue#9807 DRAFT Sep 19, 2024

This was referenced Sep 20, 2024

Add tests for VB runtime Do Not Review #11863

Closed

[Tracking] Current VB work (Refactoring and adding tests) #12230

Open

paul1956 added 30 commits January 8, 2025 19:02

Fix merge error

6c4be49

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

395de61

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

54315ae

…07-2nd-DRAFT

PR Feedback Don't create new exceptions.

bf4d70a

The server will now write issues as text to a public property ServerFault. The tests can then verify that there were no protocal issues except here expected.

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

dced84d

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

d4d4b6b

…07-2nd-DRAFT

Merge branch 'FixIssue#9807-2nd-DRAFT' into Shorten-Long-Lines-in-VB-…

1466e7f

…Review-after-FixIssue#9807-3rd

Sort Unauthorized Access tests together

b7ad7ae

Fix error in Progress Progress Dialog Labels Use Enum to define testing file sizes Get FileSize and URL from WebListner to allow 1 source of truth This will also allow easier testing with real file server

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

a084485

…07-2nd-DRAFT

Fix merge issues

4fed93b

Make it easier to test with real web server instead of WebListener

253b185

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

7ddc1b2

…07-2nd-DRAFT

Make FileName for Transfer match size

127c7ef

Allow skipping of tests that need server to support Unauthorized Access based on user reedentials. Make logic for file Upload detection not dependant on file name to support other Servers in testing.

Fix logic for ServerThrowsPasswordErrors

fde921e

Add support for real File Server Testing

Add ServerConfiguratio,json file to allow manual testing using privat…

3ed66c3

…e servers

Fix issues found in testing on File servers

c94a16f

Fix upload and download tests to handle both possible cancelations ex…

b674593

…ceptions in existing code.

Correct skip download test on GitHub

e8b912e

Improve code coverage

846ae50

Fix spelling error

abeb4f7

Fix default values in ServerConfiguration.json

bb1b65a

Correct default password for local testing

96e7834

Improve code coverage for upload tests Support more real File Server options

Make CreateTempFile more robust by making size a FileSize Enum

9faf31e

Prevent merge issue

0efa368

Merge branch 'master' into Code-Coverage-DownloadFile-Review-1St

efa5e04

Merge

74abf89

Fix test failures

1f5dd80

Cleanup VbFileCleanupTestBase

bbd01fe

Merge branch 'Code-Coverage-DownloadFile-Review-1St' into FixIssue#98…

3594993

…07-2nd-DRAFT

Merge

061cb10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

paul1956 commented Sep 11, 2024 •

edited by dotnet-policy-service bot

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

Tanya-Solyanik left a comment

Tanya-Solyanik Sep 13, 2024

paul1956 Sep 13, 2024

Tanya-Solyanik Sep 14, 2024

paul1956 Sep 14, 2024 •

edited

Loading

Tanya-Solyanik Sep 13, 2024

paul1956 Sep 14, 2024

Tanya-Solyanik Sep 14, 2024

paul1956 Sep 14, 2024 •

edited

Loading

Tanya-Solyanik Sep 14, 2024

paul1956 Sep 15, 2024

Tanya-Solyanik Sep 13, 2024

paul1956 Sep 14, 2024

ricardobossan Sep 16, 2024

paul1956 Sep 16, 2024

ricardobossan Sep 16, 2024

paul1956 Sep 16, 2024

paul1956 Sep 16, 2024


		Imports NetInfoAlias = System.Net.NetworkInformation

		Namespace Microsoft.VisualBasic.Devices


		Imports System.Runtime.CompilerServices

		<Assembly: InternalsVisibleTo("Microsoft.VisualBasic.Forms.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010007d1fa57c4aed9f0a32e84aa0faefd0de9e8fd6aec8f87fb03766c834c99921eb23be79ad9d5dcc1dd9ad236132102900b723cf980957fc4e177108fc607774f29e8320e92ea05ece4e821c0a5efe8f1645c4c0c93c1ab99285d622caa652c1dfad63d745d6f2de5f17e5eaf0fc4963d261c8a12436518206dc093344d5ad293")>

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

Are you sure you want to change the base?

Shorten long lines in VB REVIEW (3rd) AFTER #12489 DRAFT #12118

Conversation

paul1956 commented Sep 11, 2024 • edited by dotnet-policy-service bot Loading

Proposed changes

Customer Impact

Regression?

Risk

Microsoft Reviewers: Open in CodeFlow

codecov bot commented Sep 11, 2024 • edited Loading

Codecov Report

Tanya-Solyanik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul1956 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul1956 Sep 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul1956 commented Sep 11, 2024 •

edited by dotnet-policy-service bot

Loading

codecov bot commented Sep 11, 2024 •

edited

Loading

paul1956 Sep 14, 2024 •

edited

Loading

paul1956 Sep 14, 2024 •

edited

Loading