From e7f2578ce533c0e49d1f6f55f67e153f87dc25bd Mon Sep 17 00:00:00 2001 From: campersau Date: Mon, 30 Sep 2024 15:53:41 +0200 Subject: [PATCH] Implement IAsyncDisposable / IDisposable on IBrowserContext (#2786) * Implement IAsyncDisposable / IDisposable on IBrowserContext * Use IBrowserContext.DisposeAsync in tests --- .../BrowserContextOverridePermissionsTests.cs | 4 +--- .../BrowserContextTests.cs | 18 +++++---------- .../CookiesTests/SetCookiesTests.cs | 4 +--- lib/PuppeteerSharp/BrowserContext.cs | 23 +++++++++++++++++++ lib/PuppeteerSharp/IBrowserContext.cs | 2 +- 5 files changed, 32 insertions(+), 19 deletions(-) diff --git a/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextOverridePermissionsTests.cs b/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextOverridePermissionsTests.cs index 900772630..f9d0dc152 100644 --- a/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextOverridePermissionsTests.cs +++ b/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextOverridePermissionsTests.cs @@ -87,7 +87,7 @@ await Page.EvaluateFunctionAsync(@"() => { public async Task ShouldIsolatePermissionsBetweenBrowserContexts() { await Page.GoToAsync(TestConstants.EmptyPage); - var otherContext = await Browser.CreateBrowserContextAsync(); + await using var otherContext = await Browser.CreateBrowserContextAsync(); var otherPage = await otherContext.NewPageAsync(); await otherPage.GoToAsync(TestConstants.EmptyPage); Assert.That(await GetPermissionAsync(Page, "geolocation"), Is.EqualTo("prompt")); @@ -101,8 +101,6 @@ public async Task ShouldIsolatePermissionsBetweenBrowserContexts() await Context.ClearPermissionOverridesAsync(); Assert.That(await GetPermissionAsync(Page, "geolocation"), Is.EqualTo("prompt")); Assert.That(await GetPermissionAsync(otherPage, "geolocation"), Is.EqualTo("granted")); - - await otherContext.CloseAsync(); } [Test, Ignore("Fails on Firefox")] diff --git a/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextTests.cs b/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextTests.cs index e4f615176..8cc7bd047 100644 --- a/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextTests.cs +++ b/lib/PuppeteerSharp.Tests/BrowserContextTests/BrowserContextTests.cs @@ -47,7 +47,7 @@ public async Task ShouldCloseAllBelongingTargetsOnceClosingContext() [Test, Retry(2), PuppeteerTest("browsercontext.spec", "BrowserContext", "window.open should use parent tab context")] public async Task WindowOpenShouldUseParentTabContext() { - var context = await Browser.CreateBrowserContextAsync(); + await using var context = await Browser.CreateBrowserContextAsync(); var page = await context.NewPageAsync(); await page.GoToAsync(TestConstants.EmptyPage); var popupTargetCompletion = new TaskCompletionSource(); @@ -60,13 +60,12 @@ await Task.WhenAll( var popupTarget = await popupTargetCompletion.Task; Assert.That(popupTarget.BrowserContext, Is.SameAs(context)); - await context.CloseAsync(); } [Test, Retry(2), PuppeteerTest("browsercontext.spec", "BrowserContext", "should fire target events")] public async Task ShouldFireTargetEvents() { - var context = await Browser.CreateBrowserContextAsync(); + await using var context = await Browser.CreateBrowserContextAsync(); var events = new List(); context.TargetCreated += (_, e) => events.Add("CREATED: " + e.Target.Url); context.TargetChanged += (_, e) => events.Add("CHANGED: " + e.Target.Url); @@ -82,7 +81,6 @@ public async Task ShouldFireTargetEvents() $"CHANGED: {TestConstants.EmptyPage}", $"DESTROYED: {TestConstants.EmptyPage}" })); - await context.CloseAsync(); } [Test, Retry(2), PuppeteerTest("browsercontext.spec", "BrowserContext", "should isolate localStorage and cookies")] @@ -135,7 +133,7 @@ await page2.EvaluateExpressionAsync(@"{ public async Task ShouldWorkAcrossSessions() { Assert.That(Browser.BrowserContexts(), Has.Exactly(1).Items); - var context = await Browser.CreateBrowserContextAsync(); + await using var context = await Browser.CreateBrowserContextAsync(); Assert.That(Browser.BrowserContexts(), Has.Length.EqualTo(2)); var remoteBrowser = await Puppeteer.ConnectAsync(new ConnectOptions @@ -145,7 +143,6 @@ public async Task ShouldWorkAcrossSessions() var contexts = remoteBrowser.BrowserContexts(); Assert.That(contexts, Has.Length.EqualTo(2)); remoteBrowser.Disconnect(); - await context.CloseAsync(); } [Test, Retry(2), PuppeteerTest("browsercontext.spec", "BrowserContext", "should provide a context id")] @@ -154,32 +151,29 @@ public async Task ShouldProvideAContextId() Assert.That(Browser.BrowserContexts(), Has.Exactly(1).Items); Assert.That(Browser.BrowserContexts()[0].Id, Is.Null); - var context = await Browser.CreateBrowserContextAsync(); + await using var context = await Browser.CreateBrowserContextAsync(); Assert.That(Browser.BrowserContexts(), Has.Length.EqualTo(2)); Assert.That(Browser.BrowserContexts()[1].Id, Is.Not.Null); - await context.CloseAsync(); } [Test, Retry(2), PuppeteerTest("browsercontext.spec", "BrowserContext", "should wait for a target")] public async Task ShouldWaitForTarget() { - var context = await Browser.CreateBrowserContextAsync(); + await using var context = await Browser.CreateBrowserContextAsync(); var targetPromise = context.WaitForTargetAsync((target) => target.Url == TestConstants.EmptyPage); var page = await context.NewPageAsync(); await page.GoToAsync(TestConstants.EmptyPage); var promiseTarget = await targetPromise; var targetPage = await promiseTarget.PageAsync(); Assert.That(page, Is.EqualTo(targetPage)); - await context.CloseAsync(); } [Test, Retry(2), PuppeteerTest("browsercontext.spec", "BrowserContext", "should timeout waiting for a non-existent target")] public async Task ShouldTimeoutWaitingForNonExistentTarget() { - var context = await Browser.CreateBrowserContextAsync(); + await using var context = await Browser.CreateBrowserContextAsync(); Assert.ThrowsAsync(() => context.WaitForTargetAsync((target) => target.Url == TestConstants.EmptyPage, new WaitForOptions(1))); - await context.CloseAsync(); } } } diff --git a/lib/PuppeteerSharp.Tests/CookiesTests/SetCookiesTests.cs b/lib/PuppeteerSharp.Tests/CookiesTests/SetCookiesTests.cs index 0c0df1106..c7fa517fa 100644 --- a/lib/PuppeteerSharp.Tests/CookiesTests/SetCookiesTests.cs +++ b/lib/PuppeteerSharp.Tests/CookiesTests/SetCookiesTests.cs @@ -26,7 +26,7 @@ await Page.SetCookieAsync(new CookieParam [Test, Retry(2), PuppeteerTest("cookies.spec", "Cookie specs Page.setCookie", "should isolate cookies in browser contexts")] public async Task ShouldIsolateCookiesInBrowserContexts() { - var anotherContext = await Browser.CreateBrowserContextAsync(); + await using var anotherContext = await Browser.CreateBrowserContextAsync(); var anotherPage = await anotherContext.NewPageAsync(); await Page.GoToAsync(TestConstants.EmptyPage); @@ -53,8 +53,6 @@ await anotherPage.SetCookieAsync(new CookieParam Assert.That(cookies1[0].Value, Is.EqualTo("page1value")); Assert.That(cookies2[0].Name, Is.EqualTo("page2cookie")); Assert.That(cookies2[0].Value, Is.EqualTo("page2value")); - - await anotherContext.CloseAsync(); } [Test, Retry(2), PuppeteerTest("cookies.spec", "Cookie specs Page.setCookie", "should set multiple cookies")] diff --git a/lib/PuppeteerSharp/BrowserContext.cs b/lib/PuppeteerSharp/BrowserContext.cs index ffaa58dfb..0073afc87 100644 --- a/lib/PuppeteerSharp/BrowserContext.cs +++ b/lib/PuppeteerSharp/BrowserContext.cs @@ -51,10 +51,33 @@ public Task WaitForTargetAsync(Func predicate, WaitForOp /// public abstract Task ClearPermissionOverridesAsync(); + /// + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } + + /// + /// Closes the browser context. All the targets that belong to the browser context will be closed. + /// + /// ValueTask. + public async ValueTask DisposeAsync() + { + await CloseAsync().ConfigureAwait(false); + GC.SuppressFinalize(this); + } + internal void OnTargetCreated(Browser browser, TargetChangedArgs args) => TargetCreated?.Invoke(browser, args); internal void OnTargetDestroyed(Browser browser, TargetChangedArgs args) => TargetDestroyed?.Invoke(browser, args); internal void OnTargetChanged(Browser browser, TargetChangedArgs args) => TargetChanged?.Invoke(browser, args); + + /// + /// Closes the browser context. All the targets that belong to the browser context will be closed. + /// + /// Indicates whether disposal was initiated by operation. + protected virtual void Dispose(bool disposing) => _ = CloseAsync(); } } diff --git a/lib/PuppeteerSharp/IBrowserContext.cs b/lib/PuppeteerSharp/IBrowserContext.cs index b037525f5..3b60dbe6a 100644 --- a/lib/PuppeteerSharp/IBrowserContext.cs +++ b/lib/PuppeteerSharp/IBrowserContext.cs @@ -8,7 +8,7 @@ namespace PuppeteerSharp /// BrowserContexts provide a way to operate multiple independent browser sessions. When a browser is launched, it has /// a single used by default. The method creates a in the default . /// - public interface IBrowserContext + public interface IBrowserContext : IDisposable, IAsyncDisposable { /// /// Raised when the url of a target changes