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

PropertyGridView crashes on editing ListBoxProperty with null values #3008

Conversation

@M-Lipin
Copy link
Member

M-Lipin commented Mar 23, 2020

Fixes #2316

Proposed changes

  • Adding the test selected item for null before accessing the selected item accessible object in PropertyGrid's ListBox property.

Customer Impact

  • Users will not experience error even the ListBox property has
    null values.

Regression?

  • Yes

Risk

  • Minimal

Screenshots

Before

Application crash. Error with callstack ( please see #2316 )

After

No error.

Test methodology

  • Manual testing;
  • Unit test (to be implemented);
  • UI automation.

Accessibility testing

Test environment(s)

.NET Core SDK (reflecting any global.json):
Version: 5.0.100-alpha.1.20073.10
Commit: 29f4d693a9

Runtime Environment:
OS Name: Windows
OS Version: 10.0.18363
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\5.0.100-alpha1-05536\

Host (useful for support):
Version: 5.0.0-alpha.1.20072.3
Commit: c3dc1fdfdc

Previous PR (was cancelled): #2789

Microsoft Reviewers: Open in CodeFlow
…ected item is null.
@M-Lipin M-Lipin requested a review from dotnet/dotnet-winforms as a code owner Mar 23, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #3008 into master will increase coverage by 0.03166%.
The diff coverage is 91.30435%.

@@                 Coverage Diff                 @@
##              master       #3008         +/-   ##
===================================================
+ Coverage   62.14766%   62.17933%   +0.03166%     
===================================================
  Files           1257        1259          +2     
  Lines         449428      449450         +22     
  Branches       39227       39227                 
===================================================
+ Hits          279309      279465        +156     
+ Misses        164638      164514        -124     
+ Partials        5481        5471         -10
Flag Coverage Δ
#Debug 62.17933% <91.30435%> (+0.03166%) ⬆️
#production 33.38753% <0%> (+0.0456%) ⬆️
#test 98.98465% <95.45455%> (+0.00974%) ⬆️
}
}

public class Entity

This comment has been minimized.

Copy link
@RussKie

RussKie Mar 24, 2020

Member

Move within PropertyGridViewTests class. I'm quite sure it can also be made private.

[WinFormsFact]
public void PropertyGridView_Constructor()
{
var propertyGrid = new PropertyGrid();

This comment has been minimized.

Copy link
@RussKie

RussKie Mar 24, 2020

Member
Suggested change
var propertyGrid = new PropertyGrid();
using var propertyGrid = new PropertyGrid();
var item = propertyGrid.SelectedGridItem;

Assert.NotNull(item);
Assert.NotNull(item.Value);

This comment has been minimized.

Copy link
@RussKie

RussKie Mar 24, 2020

Member

Maybe test like this?

Suggested change
Assert.NotNull(item.Value);
Assert.Same(entity, item.Value);
Assert.Null(dropDownListBox.SelectedItem);

// Verify that invoking SetListBoxItemFocus does not lead to exception throwing even if SelectedItem = null.
type.InvokeMember("SetListBoxItemFocus",

This comment has been minimized.

Copy link
@RussKie

RussKie Mar 24, 2020

Member

SetListBoxItemFocus method is internal. Why do we need to invoke it via the reflection?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.