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
Closed

Add the case MauiLabelTests #2293

wants to merge 2 commits into from

Conversation

Amy-Li02-zz
Copy link
Contributor

@Amy-Li02-zz Amy-Li02-zz commented Nov 6, 2019

Porting a Winforms Maui test MauiLabelTests to xUnit

Microsoft Reviewers: Open in CodeFlow

@Amy-Li02-zz Amy-Li02-zz requested a review from a team as a code owner November 6, 2019 08:42
@dnfclas
Copy link

dnfclas commented Nov 6, 2019

CLA assistant check
All CLA requirements met.

@codecov
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
Copy link
Contributor

hughbe commented Nov 6, 2019

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

@Amy-Li02-zz
Copy link
Contributor Author

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

@AdamYoblick
Copy link
Member

AdamYoblick commented Nov 7, 2019 via email

@RussKie
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.

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

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

Copy link
Contributor Author

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.

Comment on lines 132 to 147
// 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;
Copy link
Member

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

Copy link
Contributor Author

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)
Copy link
Member

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.

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

Copy link
Contributor Author

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?

Copy link
Member

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:

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

Copy link
Member

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?

Copy link
Member

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)
Copy link
Member

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.

Copy link
Contributor Author

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]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[Theory]
[WinFormsTheory]

@RussKie RussKie added test-enhancement Improvements of test source code 📭 waiting-author-feedback The team requires more information from the author waiting-review This item is waiting on review by one or more members of team labels Nov 7, 2019
@msftbot msftbot bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 7, 2019
{
public class MauiLabelTests : ReflectBase
{
Label lbl;
Copy link
Member

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:

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

Copy link
Contributor Author

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)
Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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");
Copy link
Member

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";
Copy link
Member

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?

Copy link
Contributor Author

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)
Copy link
Member

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?

Winforms.sln Show resolved Hide resolved
@msftbot msftbot bot added the 📭 waiting-author-feedback The team requires more information from the author label Nov 7, 2019
lbl = new Label();
lbl.AutoSize = true;
lbl.Text = "Hello";
this.Controls.Add(lbl);
Copy link
Member

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

Copy link
Contributor Author

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.

@msftbot msftbot bot removed the 📭 waiting-author-feedback The team requires more information from the author label Nov 8, 2019
@AdamYoblick
Copy link
Member

AdamYoblick commented Nov 8, 2019 via email

@Amy-Li02-zz
Copy link
Contributor Author

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"/>
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they already exist.

Copy link
Member

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 😄

Copy link
Contributor Author

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)
Copy link
Member

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)
Copy link
Member

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)
Copy link
Member

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)
Copy link
Member

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

@AdamYoblick
Copy link
Member

AdamYoblick commented Nov 12, 2019 via email

@Amy-Li02-zz
Copy link
Contributor Author

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.

@RussKie RussKie deleted the Add_MauiLabelTests branch April 22, 2020 00:55
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Feb 3, 2022
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Oct 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
test-enhancement Improvements of test source code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants