-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Stop ILSpy.Tests from retaining every test's app graph (15 GB -> 0.7 GB) #4012
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
base: master
Are you sure you want to change the base?
Changes from all commits
e12677a
e5a1fd7
c0a74fc
7f0afb1
2881d8c
eefef96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,14 +17,16 @@ | |
| // DEALINGS IN THE SOFTWARE. | ||
|
|
||
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Linq; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
|
|
||
| using Avalonia; | ||
| using Avalonia.Controls.ApplicationLifetimes; | ||
| using Avalonia.Controls; | ||
| using Avalonia.Threading; | ||
| using Avalonia.VisualTree; | ||
|
|
||
| using ICSharpCode.ILSpyX.Settings; | ||
|
|
||
|
|
@@ -92,6 +94,13 @@ public void AfterTest(ITest test) | |
| if (Application.Current == null || !Dispatcher.UIThread.CheckAccess()) | ||
| return; | ||
|
|
||
| TearDownTestState(); | ||
| } | ||
|
|
||
| // Everything the per-test teardown does on the dispatcher thread; exposed so a test can | ||
| // perform the teardown itself and check what it leaves behind (see TeardownRetentionTests). | ||
| internal static void TearDownTestState() | ||
| { | ||
| // Drive background work to quiescence BEFORE the next test rebuilds the composition. A test | ||
| // that triggers a decompile spawns a Task.Run plus dispatcher continuations and rarely awaits | ||
| // them to completion; left running, that continuation lands during the next test and reads | ||
|
|
@@ -100,15 +109,50 @@ public void AfterTest(ITest test) | |
| DrainPendingWork(); | ||
|
|
||
| // Close any windows the test showed so their view-models (alive and weakly subscribed to | ||
| // MessageBus) can't react to events raised by later tests, then drain once more. | ||
| if (Application.Current.ApplicationLifetime is IClassicDesktopStyleApplicationLifetime desktop) | ||
| // MessageBus) can't react to events raised by later tests, then drain once more. A window | ||
| // left open also outlives its container: the compositor keeps every open top level | ||
| // reachable, and with it the view-models, the assembly tree and the loaded assemblies - | ||
| // about 13 MB per test, which over the suite is what pushed the CI runner into paging. | ||
| foreach (var window in openWindows.ToArray()) | ||
| { | ||
| foreach (var window in desktop.Windows.ToArray()) | ||
| window.Close(); | ||
| DetachFlyouts(window); | ||
| window.Close(); | ||
| } | ||
| Dispatcher.UIThread.RunJobs(); | ||
| } | ||
|
|
||
| // Avalonia's Button subscribes to its flyout's Opened/Closed events when its template is | ||
| // applied and unsubscribes only when the Flyout property changes, not when the button leaves | ||
| // the tree. Dock's ToolChromeControl theme gives every tool pane's chrome button the same | ||
| // MenuFlyout resource, so that one shared flyout would keep the visual tree of every window | ||
| // this suite ever showed alive. Clearing the property before the window closes is what | ||
| // makes the button let go. | ||
| static void DetachFlyouts(Window window) | ||
| { | ||
| foreach (var button in window.GetVisualDescendants().OfType<Button>()) | ||
| { | ||
| if (button.Flyout != null) | ||
| button.Flyout = null; | ||
| } | ||
| } | ||
|
|
||
| // The headless host runs the app without an application lifetime, so nothing tracks the | ||
| // windows the tests show. These are the same class handlers ClassicDesktopStyleApplicationLifetime | ||
| // installs to maintain its Windows list. | ||
| static readonly List<Window> openWindows = new(); | ||
|
|
||
| static ResetAppStateAttribute() | ||
| { | ||
| Window.WindowOpenedEvent.AddClassHandler(typeof(Window), (sender, _) => { | ||
| if (sender is Window window && !openWindows.Contains(window)) | ||
| openWindows.Add(window); | ||
| }); | ||
| Window.WindowClosedEvent.AddClassHandler(typeof(Window), (sender, _) => { | ||
| if (sender is Window window) | ||
| openWindows.Remove(window); | ||
| }); | ||
| } | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. None of the four leak fixes has a regression test, which is at odds with the repo's TDD rule for new behaviour. Concretely: if someone later drops A single cheap guard covers most of it: hold a |
||
|
|
||
| static void DrainPendingWork() | ||
| { | ||
| Task quiesce; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| using Avalonia.Controls; | ||
| using Avalonia.Headless.NUnit; | ||
| using Avalonia.Media; | ||
| using Avalonia.Threading; | ||
|
|
||
| using AwesomeAssertions; | ||
|
|
||
|
|
@@ -115,7 +116,19 @@ public async Task SearchPane_Hosts_A_Progress_Indicator_Bound_To_IsSearching() | |
| var progress = pane.FindControl<ProgressBar>("SearchProgress"); | ||
| ((object?)progress).Should().NotBeNull( | ||
| "the pane must host a progress indicator the user can see while a search runs"); | ||
| progress!.IsIndeterminate.Should().BeTrue( | ||
| "the indicator runs in indeterminate mode — we don't know the total work up front"); | ||
|
|
||
| // Indeterminate mode is tied to the search, not switched on permanently: the indicator | ||
| // is an infinite animation, and one that ran while idle would keep the render clock | ||
| // busy for as long as the pane exists. | ||
| var search = AppComposition.Current.GetExport<SearchPaneModel>(); | ||
| pane.DataContext.Should().BeSameAs(search, "the indicator binds to the pane's own model; anything else makes the assertions below meaningless"); | ||
| progress!.IsIndeterminate.Should().BeFalse("nothing is running yet"); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion is now vacuous in the failure case it is supposed to catch. Add |
||
| search.IsSearching = true; | ||
| Dispatcher.UIThread.RunJobs(); | ||
| progress.IsIndeterminate.Should().BeTrue( | ||
| "the indicator runs in indeterminate mode while a search is in flight - we don't know the total work up front"); | ||
| search.IsSearching = false; | ||
| Dispatcher.UIThread.RunJobs(); | ||
| progress.IsIndeterminate.Should().BeFalse("the animation stops with the search"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| // Copyright (c) 2026 Christoph Wille | ||
| // | ||
| // Permission is hereby granted, free of charge, to any person obtaining a copy of this | ||
| // software and associated documentation files (the "Software"), to deal in the Software | ||
| // without restriction, including without limitation the rights to use, copy, modify, merge, | ||
| // publish, distribute, sublicense, and/or sell copies of the Software, and to permit persons | ||
| // to whom the Software is furnished to do so, subject to the following conditions: | ||
| // | ||
| // The above copyright notice and this permission notice shall be included in all copies or | ||
| // substantial portions of the Software. | ||
| // | ||
| // THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, | ||
| // INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR | ||
| // PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE | ||
| // FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR | ||
| // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER | ||
| // DEALINGS IN THE SOFTWARE. | ||
|
|
||
| using System; | ||
| using System.Linq; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Threading.Tasks; | ||
|
|
||
| using Avalonia.Headless; | ||
| using Avalonia.Headless.NUnit; | ||
| using Avalonia.Threading; | ||
|
|
||
| using ICSharpCode.ILSpy.AppEnv; | ||
| using ICSharpCode.ILSpy.AssemblyTree; | ||
| using ICSharpCode.ILSpy.Views; | ||
|
|
||
| using NUnit.Framework; | ||
|
|
||
| namespace ICSharpCode.ILSpy.Tests; | ||
|
|
||
| // Most tests in this suite show a MainWindow, and the per-test teardown closes it and rebuilds | ||
| // the composition container. Anything that still reaches a closed window - a static event, a | ||
| // shared XAML resource with a subscriber, an animation on the render clock, the app-level menu - | ||
| // keeps that test's whole app graph (view-models, tree, loaded assemblies; about 13 MB) alive | ||
| // for the rest of the run, and over the suite that is enough to push a 16 GB CI runner into | ||
| // paging. Rather than asserting the absence of each known anchor, this test performs the | ||
| // teardown itself and checks that the window is actually collectable afterwards. | ||
| [TestFixture] | ||
| public class TeardownRetentionTests | ||
| { | ||
| [AvaloniaTest] | ||
| public async Task A_Main_Window_Closed_By_The_Teardown_Is_Collectable() | ||
| { | ||
| var window = ShowMainWindow(); | ||
| // Let the assembly loads the window started run to completion first: each one posts its | ||
| // completion to the dispatcher, and one posted after the teardown would hold the tree (and | ||
| // with it the window) until the next test pumps it - a false positive, not retention. | ||
| await Waiters.WaitForAsync(static () => AllAssembliesLoaded()); | ||
|
|
||
| ResetAppStateAttribute.TearDownTestState(); | ||
| // What the next test's BeforeTest does: the fresh container drops the [Shared] MainWindow. | ||
| AppComposition.CreateContainer(); | ||
|
|
||
| // The closed window's final composition batch (its target's disposal) references it until | ||
| // the compositor has committed and rendered it, and commits are throttled behind the | ||
| // previous batch's completion, which comes back through the thread pool - so keep pumping | ||
| // the dispatcher (and the headless render loop, which only ticks on request) while polling. | ||
| await Waiters.WaitForAsync(() => IsCollected(window), TimeSpan.FromSeconds(10), | ||
| "the closed MainWindow to become unreachable once its container is gone"); | ||
| } | ||
|
|
||
| static bool IsCollected(WeakReference window) | ||
| { | ||
| AvaloniaHeadlessPlatform.ForceRenderTimerTick(); | ||
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); | ||
| GC.Collect(); | ||
| return !window.IsAlive; | ||
| } | ||
|
|
||
| static bool AllAssembliesLoaded() | ||
| { | ||
| var assemblies = AppComposition.Current.GetExport<AssemblyTreeModel>().AssemblyList?.GetAssemblies(); | ||
| return assemblies is { Length: > 0 } && assemblies.All(a => a.IsLoaded); | ||
| } | ||
|
|
||
| // The window must not be referenced from this test's own frame while the GC runs. | ||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| static WeakReference ShowMainWindow() | ||
| { | ||
| var window = AppComposition.Current.GetExport<MainWindow>(); | ||
| window.Show(); | ||
| Dispatcher.UIThread.RunJobs(); | ||
| return new WeakReference(window); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two robustness gaps in this loop.
Nothing closes an open menu before the window dies.
DetachFlyoutscoversButton.Flyout, but aContextMenuleft open by a test keepsDefaultMenuInteractionHandlersubscribed to the process-globalInputManager.Instance.Process, and that handler holdsMenu.TopLevel- i.e. this window and everything under it - for the rest of the run. Several tests here open a tree context menu and never dismiss it. Closing menus in the same pass is a one-line extension and removes a retention root of the same size as the flyout one:No error containment. If any
Closing/Closedhandler throws for the first window (MainWindow.OnClosingguards its ownSaveLayout, but viewOnDetachedFromVisualTreehandlers are not guarded), theforeachaborts: every remaining window stays open and stays in the staticopenWindowslist for the whole run - the exact retention this PR removes - and the exception surfaces as a teardown error on a test that otherwise passed. Wrap the body in try/catch andopenWindows.Remove(window)unconditionally so the tracking list can never become the thing that retains a window.