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 Menu tests #310

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@hughbe
Copy link
Contributor

hughbe commented Jan 10, 2019

No description provided.

@hughbe hughbe requested a review from dotnet/dotnet-winforms as a code owner Jan 10, 2019

[Fact]
public void ContextMenu_Ctor_NullItemInMenuItemArray_ThrowsNullReferenceException()
{
Assert.Throws<NullReferenceException>(() => new ContextMenu(new MenuItem[] { null }));

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ArgumentNullException?

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

@Tanya-Solyanik is changing that kind of thing too breaking of a change?

This comment has been minimized.

@AdamYoblick

AdamYoblick Jan 14, 2019

Member

I would absolutely love if this was fixed, but it's up to the rest of the team. Allowing the framework to throw a null reference exception is never the right thing to do.

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

Let's see what @Tanya-Solyanik thinks as well; I am tempted to agree with you Adam.

This comment has been minimized.

@zsd4yr

zsd4yr Jan 16, 2019

Member

@hughbe talked to the team and we decided it is preferable to change these to throw the proper exception (for all the cases in this PR). Would you like to create an issue for that and / or take it on?

[Fact]
public void MainMenu_Ctor_NullItemInMenuItemArray_ThrowsNullReferenceException()
{
Assert.Throws<NullReferenceException>(() => new MainMenu(new MenuItem[] { null }));

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ArgumentNullException?

{
var menuItem = new MenuItem();
menuItem.Dispose();
Assert.Throws<NullReferenceException>(() => menuItem.BarBreak);

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth throwing ObjectDisposedException?

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Ditto with all the other cases in this file

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

@AdamYoblick do you feel like NullReferenceException is the incorrect error here? The object has been disposed, but it's it still operated on via a reference? My understanding is that ObjectDisposedException is for trying to operate on the actual value of the object.

This comment has been minimized.

@AdamYoblick

AdamYoblick Jan 14, 2019

Member

I agree with Hugh. ObjectDisposedException should be thrown after trying to access a disposed object.

Allowing your code to throw a NullReferenceException is almost never the right thing to do. Even the MSDN docs say it typically reflects developer error.

I'm personally fine with changing it, but I think the rest of the team won't be, probably because it's a "breaking change"

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

I'll add to tactics notes, hopefully we can figure something out

This comment has been minimized.

@zsd4yr

zsd4yr Jan 16, 2019

Member

As mentioned above, it is preferable to change to throw the expected exception instead of NullRef

destinationCallCount++;
Assert.Equal(source, sender);
Assert.Equal(EventArgs.Empty, e);
};

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ArgumentNullException?

destinationCallCount++;
Assert.Equal(source, sender);
Assert.Equal(EventArgs.Empty, e);
};

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ObjectDisposedException?

[Fact]
public void Menu_Ctor_NullItemInMenuItemArray_ThrowsNullReferenceException()
{
Assert.Throws<NullReferenceException>(() => new SubMenu(new MenuItem[] { null }));

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ArgumentNullException?

public void Menu_MergeMenu_NullSource_ThrowsNullReferenceException()
{
var menu = new SubMenu(new MenuItem[0]);
Assert.Throws<NullReferenceException>(() => menu.MergeMenu(null));

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ArgumentNullException?

public void Menu_CloneMenu_NullMenuSource_ThrowsNullReferenceException()
{
var menu = new SubMenu(new MenuItem[0]);
Assert.Throws<NullReferenceException>(() => menu.CloneMenu(null));

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Potential bug, is it worth fixing and throwing ArgumentNullException?

{
Assert.Equal(expected.Select(m => m.Text), actual.Cast<MenuItem>().Select(m => m.Text));
}
catch (NullReferenceException)

This comment has been minimized.

@hughbe

hughbe Jan 10, 2019

Contributor

Bug here, where cloning certain menu items causes the item to have null data.

@hughbe hughbe force-pushed the hughbe:add-menu-tests branch from b291bcf to 35f886d Jan 10, 2019

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Jan 10, 2019

I can obv. approve these tests; however, if you want to create an issue proposing changing some of these throws, others on the team will have to discuss that with you. I do not own enough of this lower level stuff to say for sure if it is alright to change these.

[Fact]
public void ContextMenu_Ctor_NullItemInMenuItemArray_ThrowsNullReferenceException()
{
Assert.Throws<NullReferenceException>(() => new ContextMenu(new MenuItem[] { null }));

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

@Tanya-Solyanik is changing that kind of thing too breaking of a change?

var collection = new Menu.MenuItemCollection(menu);
MenuItem menuItem = collection.Add(caption, onClick);
Assert.Same(menuItem, Assert.Single(collection));
Assert.Equal(caption ?? "", menuItem.Text);

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

in what cases is the text not set to caption? why?

This comment has been minimized.

@AdamYoblick

AdamYoblick Jan 14, 2019

Member

Nit: please use string.Empty instead of ""

MenuItem menuItem = collection.Add(caption, items);
Assert.Same(menuItem, Assert.Single(collection));
Assert.Equal(caption ?? "", menuItem.Text);
Assert.Equal(items ?? new MenuItem[0], menuItem.MenuItems.Cast<MenuItem>());

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

in what cases is this not the case?

var menuItem = new MenuItem { Name = "name" };
var menu = new SubMenu(new MenuItem[] { menuItem });
var collection = new Menu.MenuItemCollection(menu);
Assert.Equal(expected ? menuItem : null, collection[key]);

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

this is a little confusing, consider splitting into tests where "expected" is true and where it is false

This comment has been minimized.

@AdamYoblick

AdamYoblick Jan 14, 2019

Member

I'm actually ok with this, this is one of the ways inline data can save you from writing more tests. Looks good to me :)

{
var menuItem = new MenuItem();
menuItem.Dispose();
Assert.Throws<NullReferenceException>(() => menuItem.BarBreak);

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

@AdamYoblick do you feel like NullReferenceException is the incorrect error here? The object has been disposed, but it's it still operated on via a reference? My understanding is that ObjectDisposedException is for trying to operate on the actual value of the object.


public static IEnumerable<object[]> OnPopup_MdiChildren_TestData()
{
var formWithNoMdiChildren = new Form { Menu = new MainMenu() };

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

It may be helpful to add comments above each of these objects indicating what equivalence class they represent / why you chose to add them instead of relying on one of the others

This comment has been minimized.

@AdamYoblick

AdamYoblick Jan 14, 2019

Member

See my previous comments. NullReferenceException is not the right thing to throw here imo, but the team will probably not want to change it.

{
try
{
parent?.MenuItems.Add(menuItem);

This comment has been minimized.

@zsd4yr

zsd4yr Jan 14, 2019

Member

I'm not a huge fan of the parent? here. I see why you're doing this kind of thing but it makes the tests feel a little non-deterministic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment