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

Add the case MauiLabelTests #2293

Closed
wants to merge 2 commits into from

Conversation

@Amy-Li02
Copy link

Amy-Li02 commented Nov 6, 2019

Porting a Winforms Maui test MauiLabelTests to xUnit

Microsoft Reviewers: Open in CodeFlow
v-jiaol
@Amy-Li02 Amy-Li02 requested a review from dotnet/dotnet-winforms as a code owner Nov 6, 2019
@dnfclas

This comment has been minimized.

Copy link

dnfclas commented Nov 6, 2019

CLA assistant check
All CLA requirements met.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Nov 6, 2019

Codecov Report

Merging #2293 into master will increase coverage by 0.02012%.
The diff coverage is n/a.

@@                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
Flag Coverage Δ
#Debug 29.40073% <ø> (+0.02012%) ⬆️
#production 29.40073% <ø> (+0.02012%) ⬆️
#test 100% <ø> (ø) ⬆️
@hughbe

This comment has been minimized.

Copy link
Contributor

hughbe commented Nov 6, 2019

I don’t really see why these can’t be xunit tests?

@Amy-Li02

This comment has been minimized.

Copy link
Author

Amy-Li02 commented Nov 7, 2019

Hi @RussKie and @AdamYoblick, could you please take a look at this pull and approve it if the changes are ok?

@AdamYoblick

This comment has been minimized.

Copy link
Member

AdamYoblick commented Nov 7, 2019

@RussKie

This comment has been minimized.

Copy link
Member

RussKie commented Nov 7, 2019

I don’t really see why these can’t be xunit tests?

These are xUnit tests :)
We are porting a suite of existing manual integration tests, which means they may look a bit odd in places. However we're not in a position to re-write them from a scratch.

// [Scenarios]
//@ AutoTTScenario()
//@ AutoTFScenario()
//@ AutoFFScenario()
//@ AutoFTScenario()
//@ TextAlignEnumScenario()
//@ TextAlignValidLiteralScenario()
//@ TextAlignInValidLiteralScenario()
//@ TextAlignInValidLiteral2Scenario()
Comment on lines 196 to 204

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 7, 2019

Member

This is no longer required, please remove

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 7, 2019

Author

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;
Comment on lines 132 to 147

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 7, 2019

Member
            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

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 7, 2019

Author

Good idea! This change make case more concise and clear.
​​​​​​​

}

[Scenario(true)]
public ScenarioResult TextAlignValidLiteralScenario(TParams p)

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 7, 2019

Member

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.

Suggested change
public ScenarioResult TextAlignValidLiteralScenario(TParams p)
public ScenarioResult TextAlignValidIntegralScenario(TParams p)

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 7, 2019

Author

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?

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 7, 2019

Member

We can test both integral and literal scenarios:

  1. Enum.GetValues(typeof(ContentAlignment)) gives us integral values, and
  2. Enum.GetNames(typeof(ContentAlignment)) gives us names

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

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?

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 8, 2019

Member

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)

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 7, 2019

Member

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.

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 7, 2019

Author

Good idea!

{
private const string ProjectName = "MauiLabelTests";

[Theory]

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 7, 2019

Member
Suggested change
[Theory]
[WinFormsTheory]
{
public class MauiLabelTests : ReflectBase
{
Label lbl;

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

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:

  1. All members should have access modifiers (i.e. privates should explicitly say private)
  2. Private members should start with an underscore.
  3. etc...

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 8, 2019

Author

Thank you! we will modify the code to follow the naming convention.

// Test Methods
//==========================================
[Scenario(true)]
public ScenarioResult AutoTTScenario(TParams p)

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

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.

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

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?

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 8, 2019

Member

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");

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

Typo: "toggel" should be "toggle"

string descr = "";
if (NewSize.Equals(OldSize))
{
descr = "Bug # 43661";

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

What's going on here? Why are we setting the description to a bug number?

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 8, 2019

Author

This was written by a previous person, no meaning, we will modify it.

}

[Scenario(true)]
public ScenarioResult TextAlignValidLiteralScenario(TParams p)

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

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?

Winforms.sln Show resolved Hide resolved
lbl = new Label();
lbl.AutoSize = true;
lbl.Text = "Hello";
this.Controls.Add(lbl);

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 7, 2019

Member

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");

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 8, 2019

Author

Sorry for this. I added these two sentences, but I don’t know why they are missing. Will double check next time.

@AdamYoblick

This comment has been minimized.

Copy link
Member

AdamYoblick commented Nov 8, 2019

@Amy-Li02

This comment has been minimized.

Copy link
Author

Amy-Li02 commented Nov 8, 2019

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"/>

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 8, 2019

Member

I don't see this file in the PR? Does this already exist?

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 11, 2019

Author

Yes, they already exist.

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 11, 2019

Member

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 😄

This comment has been minimized.

Copy link
@Amy-Li02

Amy-Li02 Nov 12, 2019

Author

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)

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 11, 2019

Member

nit: Rename to AutoSize_Does_Not_Change_Size_When_False

}

[Scenario(true)]
public ScenarioResult TextAlign_Valid_Integral_Scenario(TParams p)

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 11, 2019

Member

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)

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 11, 2019

Member

Rename to Set_TextAlign_With_Enum_Values

}

[Scenario(true)]
public ScenarioResult TextAlign_Invalid_Integral_Scenario(TParams p)

This comment has been minimized.

Copy link
@AdamYoblick

AdamYoblick Nov 11, 2019

Member

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

@AdamYoblick

This comment has been minimized.

Copy link
Member

AdamYoblick commented Nov 12, 2019

@Amy-Li02

This comment has been minimized.

Copy link
Author

Amy-Li02 commented Nov 12, 2019

Hi @RussKie and @AdamYoblick, I opened a new pull request for this test case, please review it in another PR.
Thank you very much for reviewing my code in this PR.

@Amy-Li02 Amy-Li02 closed this Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.