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
Add the case MauiLabelTests #2293
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2293 +/- ##
==================================================
+ Coverage 29.3806% 29.40073% +0.02012%
==================================================
Files 943 945 +2
Lines 266550 266558 +8
Branches 37936 37938 +2
==================================================
+ Hits 78314 78370 +56
+ Misses 183043 182994 -49
- Partials 5193 5194 +1
|
I don’t really see why these can’t be xunit tests? |
Hi @RussKie and @AdamYoblick, could you please take a look at this pull and approve it if the changes are ok? |
These are xUnit tests :) |
// [Scenarios] | ||
//@ AutoTTScenario() | ||
//@ AutoTFScenario() | ||
//@ AutoFFScenario() | ||
//@ AutoFTScenario() | ||
//@ TextAlignEnumScenario() | ||
//@ TextAlignValidLiteralScenario() | ||
//@ TextAlignInValidLiteralScenario() | ||
//@ TextAlignInValidLiteral2Scenario() |
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.
This is no longer required, please remove
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.
Will remove these comment code snippets.
// all numeric values from ContentAlignment | ||
int[] contentAlignmentValues = { 1, 2, 4, 16, 32, 64, 256, 512, 1024 }; | ||
|
||
p.log.WriteLine("Make Sure I can set all Align Enums via literals"); | ||
lbl.AutoSize = true; | ||
int len = contentAlignmentValues.Length; | ||
try | ||
{ | ||
for (int n = 0; n < len; n++) | ||
lbl.TextAlign = (ContentAlignment)contentAlignmentValues[n]; | ||
} | ||
catch (Win32Exception) | ||
{ | ||
return new ScenarioResult(false, "Failed to set all Alignment enums"); | ||
} | ||
return ScenarioResult.Pass; |
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.
p.log.WriteLine("Make Sure I can set all ContentAlignment");
lbl.AutoSize = true;
foreach (int value in Enum.GetValues(typeof(ContentAlignment)))
{
try
{
lbl.TextAlign = (ContentAlignment)value;
}
catch
{
return new ScenarioResult(false, $"Failed to set ContentAlignment: {value}");
}
}
return ScenarioResult.Pass;
It is much better to tell what value that caused the failure, as it makes it easier to debug
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.
Good idea! This change make case more concise and clear.
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult TextAlignValidLiteralScenario(TParams p) |
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.
I don't think the name is correctly reflect the nature of the test.
Literals would be "TopLeft", "MiddleCenter", whereas here we test against integral values.
public ScenarioResult TextAlignValidLiteralScenario(TParams p) | |
public ScenarioResult TextAlignValidIntegralScenario(TParams p) |
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.
If we change this scenario as you suggest above, this name may not need to be modified or may be there is a more appropriate name, do you think?
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 can test both integral and literal scenarios:
Enum.GetValues(typeof(ContentAlignment))
gives us integral values, andEnum.GetNames(typeof(ContentAlignment))
gives us names
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.
Agree with Igor here, but I'd go one further and say the second scenario is not needed. All you're doing here is casting the enum value to the enum name, which is not testing winforms code. I think the scenario that tests Enum.GetNames is enough.
@RussKie what do you think?
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.
If we don't expect consumers to set values via literals, then we can only test integral part.
We can always add more tests, if/when it becomes necessary :)
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult TextAlignEnumScenario(TParams p) |
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.
This test looks like a duplicate of the next test TextAlignValidLiteralScenario
. Both tests suffer from the same issue - if there is a new value added to the enum, neither of them would detect the change.
My suggestion to tweak the next test addresses the shortcoming.
Ultimately this test can be removed.
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.
Good idea!
{ | ||
private const string ProjectName = "MauiLabelTests"; | ||
|
||
[Theory] |
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.
[Theory] | |
[WinFormsTheory] |
{ | ||
public class MauiLabelTests : ReflectBase | ||
{ | ||
Label lbl; |
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.
Please open this file in Visual Studio and follow whatever Roslyn analyzer suggestions are given. (See one of the exising maui tests like MauiButtonTests.cs for examples). There are many violations in this code such as:
- All members should have access modifiers (i.e. privates should explicitly say private)
- Private members should start with an underscore.
- etc...
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.
Thank you! we will modify the code to follow the naming convention.
// Test Methods | ||
//========================================== | ||
[Scenario(true)] | ||
public ScenarioResult AutoTTScenario(TParams p) |
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.
Please change the name of ALL the scenarios to something more explicit. AutoTTScenario means nothing to me. :)
For example, AutoSize_Changes_Size_When_True, something like that, in this case. It should be clear what's being tested by looking at the test name.
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.
Also, I don't think this particular test is needed, nor is the False to False toggle. Changing autosize from true to true doesn't do anything, it's already true. You should be able to just verify that the label size is changed when the text changes and autosize is true, and the label size is NOT changed when the text changes and autosize is false. Only two scenarios needed I think.
@RussKie would you agree?
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.
Yes, good pick up.
We have unit tests to ensure we don't fire dup events. The purpose of integration/behavioural tests to verify a set of components work together.
[Scenario(true)] | ||
public ScenarioResult AutoTTScenario(TParams p) | ||
{ | ||
p.log.WriteLine("Make Sure autosize works on T-T toggel"); |
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.
Typo: "toggel" should be "toggle"
string descr = ""; | ||
if (NewSize.Equals(OldSize)) | ||
{ | ||
descr = "Bug # 43661"; |
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.
What's going on here? Why are we setting the description to a bug number?
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.
This was written by a previous person, no meaning, we will modify it.
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult TextAlignValidLiteralScenario(TParams p) |
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.
Agree with Igor here, but I'd go one further and say the second scenario is not needed. All you're doing here is casting the enum value to the enum name, which is not testing winforms code. I think the scenario that tests Enum.GetNames is enough.
@RussKie what do you think?
lbl = new Label(); | ||
lbl.AutoSize = true; | ||
lbl.Text = "Hello"; | ||
this.Controls.Add(lbl); |
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.
Please check the porting doc I wrote. You are missing the following in your constructor: this.BringToForeground();
You are also missing this in your Main method:
Thread.CurrentThread.SetCulture("en-US");
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.
Sorry for this. I added these two sentences, but I don’t know why they are missing. Will double check next time.
Ah ok, please ignore my comment then :)
|
Hi @RussKie and @AdamYoblick, I have modified the code according to the results of review, could you please take a look at it again? Thank you so much! |
@@ -0,0 +1,9 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
|
|||
<Import Project="..\References.targets"/> |
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.
I don't see this file in the PR? Does this already exist?
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.
Yes, they already exist.
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.
When I check MauiButtonTests.csproj, I don't see this line. This is why I asked. I don't think it's needed, and it would not be there if the csproj was copied from an existing 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.
Hi @AdamYoblick, today I fork and clone the latest repo and double check MauiButtonTests.csproj, the above lines exist in there. If we need to remove it?
When I try to comment it, solution will build with some errors.
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult AutoSize_Not_Changes_Size_When_False(TParams p) |
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.
nit: Rename to AutoSize_Does_Not_Change_Size_When_False
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult TextAlign_Valid_Integral_Scenario(TParams p) |
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.
This name is still not very clear. I would rename to Set_TextAlign_With_Enum_Names
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult TextAlign_Valid_Literal_Scenario(TParams p) |
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.
Rename to Set_TextAlign_With_Enum_Values
} | ||
|
||
[Scenario(true)] | ||
public ScenarioResult TextAlign_Invalid_Integral_Scenario(TParams p) |
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.
There's no need for two different scenarios testing invalid TextAlign values. The test for 3 is the same for the test with -1 in this case.
Please get rid of the duplicate and rename this to Set_TextAlign_With_Invalid_Enum_Value
Hi @RussKie and @AdamYoblick, I opened a new pull request for this test case, please review it in another PR. |
Porting a Winforms Maui test MauiLabelTests to xUnit
Microsoft Reviewers: Open in CodeFlow