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

WIP: Add BaseContextMenuStripTests #2431

Open
wants to merge 1 commit into
base: master
from

Conversation

@brucremo
Copy link

brucremo commented Nov 25, 2019

Issue #721 (Partial) : Add BaseContextMenuStripTests

Microsoft Reviewers: Open in CodeFlow
@brucremo brucremo requested a review from dotnet/dotnet-winforms as a code owner Nov 25, 2019
}

[Fact]
public void BaseContextMenuStrip_RefreshItems()

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 26, 2019

Member

The name of a test must clearly state the intent of the test, e.g.

Suggested change
public void BaseContextMenuStrip_RefreshItems()
public void BaseContextMenuStrip_RefreshItems_<condition>_Throws<exception type>()

The current name (BaseContextMenuStrip_RefreshItems) tells us that we are testing BaseContextMenuStrip.RefreshItems() method but nothing else.
Further looking at the test itself, it asserts that it doesn't throw, which given the method's implementation is almost useless.

Looking at the code I can see that we have a risk of an exception on line:225 - what if uis.Styles doesn't contain an entry with DialogFont key, or the value is null?

if (serviceProvider.GetService(typeof(IUIService)) is IUIService uis)
{
Font = (Font)uis.Styles["DialogFont"];
}

We are doing a similar check here:
if (uis.Styles["VsColorPanelText"] is Color)
{
ForeColor = (Color)uis.Styles["VsColorPanelText"];
}

So we should have a test that ensures we get no exception if uis.Styles["DialogFont"] is null or otherwise unset, or is not a Font. And fix the code as part of it :)

Another test would be to verify these lines:

foreach (ToolStripItem item in Items)
{
if (item is StandardCommandToolStripMenuItem stdItem)
{
stdItem.RefreshItem();
}
}

You can subclass StandardCommandToolStripMenuItem, populate Items and assert the subclassed items have been refreshed.

And then we'd need to look what and how we can test this call:

I hope it makes sense.

Assert.NotNull(baseContextMenuStrip);
}

[Fact]

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 26, 2019

Member
Suggested change
[Fact]
[WinFormsFact]
@RussKie RussKie changed the title Issue #721 (Partial) : Add BaseContextMenuStripTests WIP: Add BaseContextMenuStripTests Nov 26, 2019
namespace System.Windows.Forms.Design.Tests
{
public class BaseContextMenuStripTests
{

This comment has been minimized.

Copy link
@RussKie

RussKie Nov 26, 2019

Member

We need to add this piece of code to each test class to help us track test failures:

        public BaseContextMenuStripTests()
        {
            Application.ThreadException += (sender, e) => throw new Exception(e.Exception.StackTrace.ToString());
        }
[Fact]
public void BaseContextMenuStrip_Ctor()
{
var mockServiceProvider = new Mock<IServiceProvider>(MockBehavior.Default);

This comment has been minimized.

Copy link
@hughbe

hughbe Nov 27, 2019

Contributor

I think we use Strict not Default

@msftbot

This comment has been minimized.

Copy link
Contributor

msftbot bot commented Dec 4, 2019

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.