-
Notifications
You must be signed in to change notification settings - Fork 984
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
base: main
Are you sure you want to change the base?
Code coverage for WindowsFormsApplicationBase #12455
Conversation
Make some Private items Friend to enable better code coverage
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. |
Add more tests for MainForm and SplashScreen
Const value As AuthenticationMode = CType(-1, AuthenticationMode) | ||
Const paramName As String = NameOf(AuthenticationMode) |
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.
Do these need to be const?
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.
@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.
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.
Now that we've made these changes, could you check if it is still giving you trouble if these are not consts?
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.
@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))> |
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.
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
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.
@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.
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.
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.
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.
@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
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.
@lonitra I did create something that works but I am not sure it is better.
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.
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
...t.VisualBasic.Forms/tests/UnitTests/System/Windows/Forms/WindowsFormsApplicationBaseTests.vb
Outdated
Show resolved
Hide resolved
...soft.VisualBasic.Forms/tests/UnitTests/System/Windows/TestUtilities/TestData/EnumTestData.vb
Outdated
Show resolved
Hide resolved
...soft.VisualBasic.Forms/tests/UnitTests/System/Windows/TestUtilities/TestData/EnumTestData.vb
Outdated
Show resolved
Hide resolved
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.
Awesome work!
Const value As AuthenticationMode = CType(-1, AuthenticationMode) | ||
Const paramName As String = NameOf(AuthenticationMode) |
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.
Now that we've made these changes, could you check if it is still giving you trouble if these are not consts?
@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. |
Improves code coverage
Proposed changes
Customer Impact
Regression?
Risk
Microsoft Reviewers: Open in CodeFlow