Skip to content

[Testing] Make tests depending on GC less flaky #28489

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.

Sign up for GitHub

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

Closed
simonrozsival wants to merge 4 commits into from
Closed

[Testing] Make tests depending on GC less flaky #28489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b77755
Improve SetterSpecificityListTests
simonrozsival Mar 18, 2025
cba741d
Move GC.Collect() + GC.WaitForPendingFinalizers() into TestHelpers
simonrozsival Mar 18, 2025
7b175a1
Do not use the helper in Windows device tests
simonrozsival Mar 18, 2025
fa51962
Increase GC count
simonrozsival Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 4 additions & 9 deletions src/Controls/tests/Core.UnitTests/BindableLayoutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ WeakReference CreateReference()

var weakReference = CreateReference();

await TestHelpers.Collect();
await TestHelpers.CollectAsync();

Assert.False(weakReference.IsAlive);

Expand Down Expand Up @@ -627,14 +627,11 @@ public async Task DoesNotLeak()

// First GC
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
await TestHelpers.CollectAsync();
Assert.False(controllerRef.IsAlive, "BindableLayoutController should not be alive!");

// Second GC
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
await TestHelpers.CollectAsync();
Assert.False(proxyRef.IsAlive, "WeakCollectionChangedProxy should not be alive!");
}

Expand All @@ -648,9 +645,7 @@ public async Task UpdatesAfterGC()

Assert.Equal(2, layout.Children.Count);

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
await TestHelpers.CollectAsync();

list.Add("Baz");
Assert.Equal(3, layout.Children.Count);
Expand Down
9 changes: 3 additions & 6 deletions src/Controls/tests/Core.UnitTests/BindingBaseUnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,7 @@ public void SourceAndTargetAreWeakWeakSimplePath(BindingMode mode)

create();

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

if (mode == BindingMode.TwoWay || mode == BindingMode.OneWay)
Assert.False(weakViewModel.IsAlive, "ViewModel wasn't collected");
Expand Down Expand Up @@ -760,8 +759,7 @@ public void CollectionAndContextAreHeldWeakly()

create();

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

Assert.False(weakCollection.IsAlive);
Assert.False(weakContext.IsAlive);
Expand Down Expand Up @@ -796,8 +794,7 @@ public void CollectionAndContextAreHeldWeaklyClosingOverCollection()

create();

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

Assert.False(weakCollection.IsAlive);
Assert.False(weakContext.IsAlive);
Expand Down
15 changes: 5 additions & 10 deletions src/Controls/tests/Core.UnitTests/BindingUnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,9 +1244,7 @@ void create()
};
create();

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
await TestHelpers.CollectAsync();

if (mode == BindingMode.TwoWay || mode == BindingMode.OneWay)
Assert.False(weakViewModel.IsAlive, "ViewModel wasn't collected");
Expand All @@ -1255,9 +1253,7 @@ void create()
Assert.False(weakBindable.IsAlive, "Bindable wasn't collected");

// WeakPropertyChangedProxy won't go away until the second GC, BindingExpressionPart unsubscribes in its finalizer
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
await TestHelpers.CollectAsync();

foreach (var proxy in proxies)
{
Expand All @@ -1279,8 +1275,7 @@ public void SourceAndTargetAreWeakComplexPath(BindingMode mode)

HackAroundMonoSucking(0, property, binding, out weakViewModel, out weakBindable);

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

if (mode == BindingMode.TwoWay || mode == BindingMode.OneWay)
Assert.False(weakViewModel.IsAlive, "ViewModel wasn't collected");
Expand Down Expand Up @@ -2196,7 +2191,7 @@ void CreateReference()

Assert.Equal(1, viewModel.InvocationListSize());

await TestHelpers.Collect();
await TestHelpers.CollectAsync();

viewModel.OnPropertyChanged("Foo");

Expand Down Expand Up @@ -2226,7 +2221,7 @@ WeakReference CreateReference()

Assert.Equal(1, viewModel.InvocationListSize());

await TestHelpers.Collect();
await TestHelpers.CollectAsync();

Assert.False(bindingRef.IsAlive, "Binding should not be alive!");

Expand Down
6 changes: 2 additions & 4 deletions src/Controls/tests/Core.UnitTests/Layouts/GridLayoutTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ WeakReference CreateReference()

WeakReference reference = CreateReference();

await TestHelpers.Collect();
await TestHelpers.CollectAsync();

Assert.False(reference.IsAlive, "Grid should not be alive!");

Expand All @@ -223,9 +223,7 @@ public async Task RowDefinitionDoesNotLeak()
reference = new(grid);
}

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
await TestHelpers.CollectAsync();

Assert.False(reference.IsAlive, "Grid should not be alive!");
}
Expand Down
10 changes: 3 additions & 7 deletions src/Controls/tests/Core.UnitTests/ListProxyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,9 +382,7 @@ public void ProxyIsWeaklyHeldByINotifyCollectionChanged()

create();

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
TestHelpers.Collect();

Assert.False(weakProxy.IsAlive);
}
Expand Down Expand Up @@ -439,15 +437,13 @@ public void WeakToWeak()

Assert.True(list.AddObject(), "GC hasn't run");

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

Assert.True(list.AddObject(), "GC run, but proxy should still hold a reference");

_proxyForWeakToWeakTest = null;

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

Assert.False(list.AddObject(), "Proxy is gone and GC has run");
}
Expand Down
3 changes: 1 addition & 2 deletions src/Controls/tests/Core.UnitTests/ListViewTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,7 @@ public void UncollectableHeaderReferences()
var header = list.TemplatedItems.GetGroup(0).HeaderContent;
}

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

// use less or equal because mono will keep the last header var alive no matter what
Assert.True(TestCell.NumberOfCells <= 6, $"{TestCell.NumberOfCells} <= 6");
Expand Down
2 changes: 1 addition & 1 deletion src/Controls/tests/Core.UnitTests/MapTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ WeakReference CreateReference()

var weakReference = CreateReference();

await TestHelpers.Collect();
await TestHelpers.CollectAsync();

Assert.False(weakReference.IsAlive);

Expand Down
17 changes: 6 additions & 11 deletions src/Controls/tests/Core.UnitTests/MessagingCenterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,7 @@ public void SubscriberShouldBeCollected()
MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => throw new XunitException("The subscriber should have been collected."));
})();

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

var pub = new TestPublisher();
pub.Test(); // Assert.Fail() shouldn't be called, because the TestSubcriber object should have ben GCed
Expand All @@ -223,8 +222,7 @@ public void ShouldBeCollectedIfCallbackTargetIsSubscriber()
subscriber.SubscribeToTestMessages();
})();

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

var pub = new TestPublisher();
pub.Test();
Expand All @@ -248,8 +246,7 @@ public void NotCollectedIfSubscriberIsNotTheCallbackTarget()
MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => subscriber.SetSuccess());
})();

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

Assert.True(wr.IsAlive); // The closure in Subscribe should be keeping the subscriber alive
Assert.NotNull(wr.Target as TestSubcriber);
Expand Down Expand Up @@ -287,7 +284,7 @@ WeakReference CreateReference()

var wr = CreateReference();

await TestHelpers.Collect();
await TestHelpers.CollectAsync();

Assert.False(wr.IsAlive); // The Action target and subscriber were the same object, so both could be collected
}
Expand All @@ -301,8 +298,7 @@ public void StaticCallback()

MessagingCenter.Subscribe<TestPublisher>(_subscriber, "test", p => MessagingCenterTestsCallbackSource.Increment(ref i));

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

var pub = new TestPublisher();
pub.Test();
Expand All @@ -320,8 +316,7 @@ public void NothingShouldBeCollected()
var source = new MessagingCenterTestsCallbackSource();
MessagingCenter.Subscribe<TestPublisher>(_subscriber, "test", p => source.SuccessCallback(ref success));

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

var pub = new TestPublisher();
pub.Test();
Expand Down
3 changes: 1 addition & 2 deletions src/Controls/tests/Core.UnitTests/MultiPageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,8 +874,7 @@ static void SurfaceGCBugs()
// the collection changed handlers in ListProxy; without the GC calls if we introduce
// a reference bug the tests will still usually pass on Debug builds and only intermittently
// fail on Release builds. We don't want these bugs to accidentally slip by.
GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();
}
}
}
3 changes: 1 addition & 2 deletions src/Controls/tests/Core.UnitTests/NavigationUnitTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -719,8 +719,7 @@ public async Task ReleasesPoppedPage(bool useMaui)

await Task.Delay(100);

GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();

Assert.True(isFinalized);
}
Expand Down
11 changes: 3 additions & 8 deletions src/Controls/tests/Core.UnitTests/PlatformBindingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,8 +200,7 @@ public PlatformBindingTests()
DispatcherProvider.SetCurrent(new DispatcherProviderStub());

//this should collect the ConditionalWeakTable
GC.Collect();
GC.WaitForPendingFinalizers();
TestHelpers.Collect();
}

public void Dispose() => DispatcherProvider.SetCurrent(null);
Expand Down Expand Up @@ -342,9 +341,7 @@ public void PlatformViewsAreCollected()

create();

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
TestHelpers.Collect();

Assert.False(wr.IsAlive);
}
Expand Down Expand Up @@ -378,9 +375,7 @@ public void ProxiesAreCollected()

create();

GC.Collect();
GC.WaitForPendingFinalizers();
GC.Collect();
TestHelpers.Collect();

Assert.False(wr.IsAlive);
}
Expand Down
16 changes: 5 additions & 11 deletions src/Controls/tests/Core.UnitTests/SetterSpecificityListTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
using System;
using System;
using System.Threading.Tasks;
using Xunit;

Expand Down Expand Up @@ -46,11 +46,8 @@ public async Task RemovingValueDoesNotLeak()

list.Remove(SetterSpecificity.FromBinding);

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(weakReference.TryGetTarget(out _));
var isAlive = await TestHelpers.WaitForCollect(weakReference);
Assert.False(isAlive);
}

[Fact]
Expand All @@ -67,11 +64,8 @@ public async Task RemovingLastValueDoesNotLeak()

list.Remove(SetterSpecificity.ManualValueSetter);

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(weakReference.TryGetTarget(out _));
var isAlive = await TestHelpers.WaitForCollect(weakReference);
Assert.False(isAlive);
}

[Fact]
Expand Down
37 changes: 33 additions & 4 deletions src/Controls/tests/Core.UnitTests/TestHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,24 @@ namespace Microsoft.Maui.Controls.Core.UnitTests
{
internal static class TestHelpers
{
public static async Task Collect()
public static void Collect(int count = 10)
{
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
for (int i = 0; i < count; i++)
{
GC.Collect();
GC.WaitForPendingFinalizers();
}
}

public static async Task CollectAsync(int count = 10)
{
for (int i = 0; i < count; i++)
{
await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();
}
}

public static async Task<bool> WaitForCollect(this WeakReference reference)
{
Expand All @@ -24,5 +35,23 @@ public static async Task<bool> WaitForCollect(this WeakReference reference)

return reference.IsAlive;
}

public static async Task<bool> WaitForCollect<T>(this WeakReference<T> reference)
where T : class
{
for (int i = 0; i < 40; i++)
{
await Task.Yield();

@MartyIX MartyIX Mar 18, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this call intentionally in the for loop? I would naively expect it to be called once.

@simonrozsival simonrozsival Mar 18, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In this case I just modified the existing WaitForCollect for WeakReference<T> and I have not considered changing it.

GC.Collect();
GC.WaitForPendingFinalizers();

if (!reference.TryGetTarget(out _))
{
return false;
}
}

return true;
}
}
}
2 changes: 1 addition & 1 deletion src/Controls/tests/Core.UnitTests/TitleBarTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ WeakReference CreateReference()
var reference = CreateReference();

// GC
await TestHelpers.Collect();
await TestHelpers.CollectAsync();

Assert.False(reference.IsAlive, "TitleBar should not be alive!");

Expand Down
Loading