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

Reduce sharing of disposable objects across MemberData sets #3487

Closed
Tracked by #8607
weltkante opened this issue Jun 22, 2020 · 3 comments
Closed
Tracked by #8607

Reduce sharing of disposable objects across MemberData sets #3487

weltkante opened this issue Jun 22, 2020 · 3 comments
Labels
help wanted Good issue for external contributors test-enhancement Improvements of test source code
Milestone

Comments

@weltkante
Copy link
Contributor

weltkante commented Jun 22, 2020

.NET Core Version:
master

Have you experienced this same bug with .NET Framework?:
no

Problem description:
During investigation of flaky unit tests it was noticed that sharing disposable objects between multiple yields of MemberData sets is not advisable. It works but relies on the undocumented fact that xunit disposes member data after all tests have run. If it were to dispose between test runs it would break those tests.

Its better style to have each set of test data be independent. This also reduces risk of undesired interaction between data sets.

Expected behavior:
Each set of (disposable) test data returned by MemberData functions should be independent

Minimal repro:
Source code review for shared member data

@ghost ghost added the 🚧 work in progress Work that is current in progress label Jun 22, 2020
@merriemcgaw merriemcgaw added the test-enhancement Improvements of test source code label Jun 25, 2020
@merriemcgaw merriemcgaw added this to the 5.0 milestone Jun 25, 2020
@ghost ghost removed the 🚧 work in progress Work that is current in progress label Jul 9, 2020
@merriemcgaw merriemcgaw modified the milestones: 5.0, 6.0 Preview1 Sep 1, 2020
@RussKie RussKie modified the milestones: 6.0 Preview1, 6.0 Preview2 Jan 27, 2021
@RussKie
Copy link
Member

RussKie commented Jan 27, 2021

I've gone through almost 2,000 MemberData methods we have in the codebase (searched for public static IEnumerable<), and it doesn't look too bad. I likely have missed few bad apples, though even if I did it won't change the result set significantly.

Here's a raw list:

OnLayout_TestData

ToolStripControlHost_ToolStripHostedControlAccessibleObjectTests
ToolStripItemAccessibleObject_TestData
PaintValue_PaintValueEventArgs_TestData
PaintValue_Object_Graphics_Rectangle_TestData


ButtonBaseTests
----------------
Enabled_Set_TestData
Enabled_SetWithHandle_TestData
Parent_Set_TestData
Visible_Set_TestData
Visible_SetWithHandle_TestData
OnEnabledChanged_TestData
OnEnabledChanged_WithHandle_TestData
OnParentChanged_TestData
OnParentChanged_WithHandle_TestData
OnVisibleChanged_TestData
OnVisibleChanged_WithHandle_TestData

ComboBoxTests
----------------
FindString_TestData
FindStringExact_TestData



OnPaintBackground_TestData
OnPaintBackground_VisualStyles_off_WithParent_TestData
OnPaintBackground_VisualStyles_on_WithParent_TestData
OnPaintBackground_WithParentWithHandle_TestData

Ctor_DataGridView_Graphics_Rectangle_Rectangle_Int_DataGridViewElementStates_String_DataGridViewCellStyle_Bool_Bool_TestData
Ctor_Graphics_TreeNode_Rectangle_TreeNodeStates_TestData

ErrorProviderTests
----------------
Icon_Set_TestData
BindToDataAndErrors_TestData


OnDrawItem_TestData

CanConvertFrom_Context_TestData
CanConvertTo_Context_TestData

Ctor_Graphics_Int_TestData

NotifyIconTests
----------------
Text_Set_TestData
Text_SetDesignMode_TestData
Dispose_WithProperties_TestData

DrawItemEventArgs_TestData

TabPageTests
----------------
OnPaintBackground_TestData
OnPaintBackground_WithParent_TestData
OnPaintBackground_WithParentWithHandle_TestData

ToolStripControlHostTests
----------------
Parent_Set_TestData
SetVisibleCore_TestData
OnParentChanged_TestData


Ctor_Graphics_ToolStrip_TestData
Ctor_Graphics_ToolStripItem_Rectangle_TestData
Ctor_Graphics_ToolStripItem_TestData

ToolStripItemTests
----------------
Available_SetWithOwnerWithHandle_TestData
Enabled_Set_TestData
Enabled_SetWithParentWithHandle_TestData
Parent_Set_TestData
Visible_SetWithOwnerWithHandle_TestData
OnEnabledChanged_TestData
OnParentChanged_TestData
OnVisibleChanged_TestData
SetVisibleCore_TestData
SetVisibleCore_InvokeWithOwnerWithHandle_TestData

ToolStripItemImageRenderEventArgs_TestData
ToolStripSeparatorRenderEventArgs_TestData
ToolStripRenderEventArgs_TestData
ToolStripPanelRenderEventArgs_TestData
ToolStripContentPanelRenderEventArgs_TestData

ToolStripSeparatorTests
----------------
Enabled_Set_TestData

ToolStripTests
----------------
OnPaintBackground_TestData
OnPaintBackground_VisualStyles_off_WithParent_TestData
OnPaintBackground_VisualStyles_on_WithParent_TestData
OnPaintBackground_WithParentWithHandle_TestData


TreeViewTests
----------------
ImageList_TestData
ItemHeight_Get_TestData

Children_GetInvalidService_TestData

@RussKie RussKie added the help wanted Good issue for external contributors label Jan 27, 2021
@RussKie RussKie modified the milestones: 6.0 Preview2, 6.0 Jan 29, 2021
@RussKie RussKie modified the milestones: 6.0, 7.0 Aug 27, 2021
@merriemcgaw merriemcgaw modified the milestones: .NET 7.0, .NET 8.0 Aug 11, 2022
@JeremyKuhne JeremyKuhne modified the milestones: .NET 8.0, .NET 9.0 Aug 16, 2023
@JeremyKuhne
Copy link
Member

@dreddy-work I'm not sure how much more we have here. I took care of some of this with the CommonMemberDataAttribute classes. We might be able to close this?

@JeremyKuhne
Copy link
Member

We can reactivate if we find any new instances of this.

@github-actions github-actions bot locked and limited conversation to collaborators Aug 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Good issue for external contributors test-enhancement Improvements of test source code
Projects
None yet
Development

No branches or pull requests

5 participants