Skip to content
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 WindowsFormsApplicationBase #12455

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

paul1956
Copy link
Contributor

@paul1956 paul1956 commented Nov 9, 2024

Improves code coverage

Proposed changes

  • Add better code coverage for WindowsFormsApplicationBase

Customer Impact

  • Improved code quality

Regression?

  • No

Risk

  • Minimal only tests and making a few Private items Friend
Microsoft Reviewers: Open in CodeFlow

Copy link

codecov bot commented Nov 9, 2024

Codecov Report

Attention: Patch coverage is 96.52778% with 5 lines in your changes missing coverage. Please review.

Project coverage is 75.71597%. Comparing base (d5bd070) to head (9e931d3).
Report is 2 commits behind head on main.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main      #12455          /-   ##
===================================================
- Coverage   75.73525%   75.71597%   -0.01928%     
===================================================
  Files           3153        3155           2     
  Lines         635807      635924         117     
  Branches       46975       46984           9     
===================================================
- Hits          481530      481496         -34     
- Misses        150842      150965         123     
- Partials        3435        3463          28     
Flag Coverage Δ
Debug 75.71597% <96.52778%> (-0.01928%) ⬇️
integration 18.17755% <33.33333%> (-0.09288%) ⬇️
production 49.25762% <88.88889%> (-0.05284%) ⬇️
test 97.05176% <98.29060%> ( 0.00069%) ⬆️
unit 46.37298% <88.88889%> ( 0.08277%) ⬆️

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

Comment on lines 120 to 121
Const value As AuthenticationMode = CType(-1, AuthenticationMode)
Const paramName As String = NameOf(AuthenticationMode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these need to be const?

Copy link
Contributor Author

@paul1956 paul1956 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonitra VB recommends they be, not sure it's required. If you are asking why are they not inlined? It makes the longs too long and this seemed cleaner than wrapping them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we've made these changes, could you check if it is still giving you trouble if these are not consts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonitra They don't need to be constants they could be Dim'ed but if I inline them the lines are too long.

Instead of declaring them (with Const or Dim) I could inline and wrap the lines if you think that would be better.

End Sub

<WinFormsTheory>
<ClassData(GetType(AuthenticationModeData))>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have an EnumDataAttribute which takes all of the values in the enum. Can that be used here? Then we won't need to add AuthenticationModeData class. Same with ShutdownModeData

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonitra I tried using them (it would be a lot simpler and also address the Const issue above) and while they worked previously I can't get the latest version of the VB compiler to resolve the reference it's like they don't exist but Intellisence suggests them while typing and I have been unable to fix it. Any help here would be appreciated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah it looks like vb doesn't support generic types in attributes unfortunately https://learn.microsoft.com/dotnet/visual-basic/misc/bc32066 I would prefer not to keep creating classes every time we want to test a set of enum values. Could you investigate creating a generic class to serve this purpose? Perhaps we can convert your implementation of AuthenticationModeData to make it generic for any enum value or try creating another EnumDataAttribute which does not take generic type and instead we pass type to its constructor.

Copy link
Contributor Author

@paul1956 paul1956 Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonitra I don't know C# well enough to create the TheoryData, and my In VB I can create the Class I don't know how to use it to create TheoryData to use in a test.

    Public Class EnumTestData
        Implements IEnumerable(Of Object())

        Private ReadOnly _enumType As Type

        Public Sub New(TEnum As Type)
            _enumType = TEnum
        End Sub

        Public Iterator Function GetEnumerator() As IEnumerator(Of Object()) Implements IEnumerable(Of Object()).GetEnumerator
            For Each mode As [Enum] In [Enum].GetValues(_enumType)
                Yield {mode}
            Next
        End Function

        Public Function IEnumerable_GetEnumerator() As IEnumerator Implements IEnumerable.GetEnumerator
            Return GetEnumerator()
        End Function
    End Class
        <WinFormsFact>
        Public Sub EnumDataIteratorTests()
            Dim testClass As New EnumTestData(GetType(AuthenticationMode))
            testClass.IEnumerable_GetEnumerator.Should.NotBeNull()
            testClass.Any.Should.BeTrue()
        End Sub

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lonitra I did create something that works but I am not sure it is better.

@lonitra lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Nov 13, 2024
@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 13, 2024
@lonitra lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Nov 13, 2024
@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 14, 2024
Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some suggestions so that we can still take advantage of using <WinFormsTheory> and <ClassData> instead of doing a foreach in all enum related tests

@lonitra lonitra added the 📭 waiting-author-feedback The team requires more information from the author label Nov 14, 2024
@dotnet-policy-service dotnet-policy-service bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 15, 2024
Copy link
Member

@lonitra lonitra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

Comment on lines 120 to 121
Const value As AuthenticationMode = CType(-1, AuthenticationMode)
Const paramName As String = NameOf(AuthenticationMode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we've made these changes, could you check if it is still giving you trouble if these are not consts?

@lonitra lonitra added the ready-to-merge PRs that are ready to merge but worth notifying the internal team. label Nov 15, 2024
@paul1956
Copy link
Contributor Author

Awesome work!

@lonitra thanks, I think I can convert the invalid Enum tests in similar way, if you want to delay merge for a day or so. The VB in WinForms does not use bit flags (though VB can with an Attribute) so the conversion is much simpler then the C# version. But if you want I could support that as well.

That would also eliminate the Constant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge PRs that are ready to merge but worth notifying the internal team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants