From 6799bdb0399f7dca76510f67f48d7e197305ecc9 Mon Sep 17 00:00:00 2001 From: Max Schmitt Date: Fri, 14 Mar 2025 17:19:21 +0100 Subject: [PATCH] chore: use manual TimeoutAttribute instead of CancelAfter attribute (#3126) --- .../Attributes/PlaywrightTestAttribute.cs | 106 ++++++++++++++---- src/Playwright.Tests/BaseTests/HttpService.cs | 4 +- .../BrowserTypeConnectTests.cs | 3 +- .../ClientCertficatesTests.cs | 5 - .../PageSetInputFilesTests.cs | 3 +- .../PageWaitForNavigationTests.cs | 3 +- src/Playwright.Tests/Playwright.Tests.csproj | 6 +- 7 files changed, 90 insertions(+), 40 deletions(-) diff --git a/src/Playwright.Tests/Attributes/PlaywrightTestAttribute.cs b/src/Playwright.Tests/Attributes/PlaywrightTestAttribute.cs index 3b59dacb..f450615a 100644 --- a/src/Playwright.Tests/Attributes/PlaywrightTestAttribute.cs +++ b/src/Playwright.Tests/Attributes/PlaywrightTestAttribute.cs @@ -22,6 +22,7 @@ * SOFTWARE. */ +using System.Reflection; using NUnit.Framework.Interfaces; using NUnit.Framework.Internal; using NUnit.Framework.Internal.Commands; @@ -36,14 +37,24 @@ namespace Microsoft.Playwright.Tests; /// Enables decorating test facts with information about the corresponding test in the upstream repository. /// [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)] -public class PlaywrightTestAttribute : TestAttribute, IApplyToContext, IApplyToTest, IWrapSetUpTearDown +public class PlaywrightTestAttribute : TestAttribute, IWrapSetUpTearDown { - private readonly CancelAfterAttribute _cancelAfterAttribute = new(TestConstants.DefaultTestTimeout); + private readonly int? _timeout; public PlaywrightTestAttribute() { } + public PlaywrightTestAttribute(int timeout) : this() + { + _timeout = timeout; + } + + public PlaywrightTestAttribute(string fileName, string nameOfTest, int timeout) : this(fileName, nameOfTest) + { + _timeout = timeout; + } + /// /// Creates a new instance of the attribute. /// @@ -60,11 +71,6 @@ public class PlaywrightTestAttribute : TestAttribute, IApplyToContext, IApplyToT /// public string FileName { get; } - /// - /// Returns the trimmed file name. - /// - public string TrimmedName => FileName.Substring(0, FileName.IndexOf('.')); - /// /// The name of the test, the decorated code is based on. /// @@ -75,22 +81,6 @@ public class PlaywrightTestAttribute : TestAttribute, IApplyToContext, IApplyToT /// public string Describe { get; } - public void ApplyToContext(TestExecutionContext context) - { - if (context.TestCaseTimeout == 0) - { - (_cancelAfterAttribute as IApplyToContext).ApplyToContext(context); - } - } - - public new void ApplyToTest(Test test) - { - base.ApplyToTest(test); - if (TestExecutionContext.CurrentContext.TestCaseTimeout == 0) - { - _cancelAfterAttribute.ApplyToTest(test); - } - } /// /// Wraps the current test command in a . /// @@ -98,6 +88,7 @@ public class PlaywrightTestAttribute : TestAttribute, IApplyToContext, IApplyToT /// the wrapped test command public TestCommand Wrap(TestCommand command) { + command = new TimeoutCommand(command, _timeout ?? TestConstants.DefaultTestTimeout); if (Environment.GetEnvironmentVariable("CI") != null) { command = new RetryCommand(command, 3); @@ -121,7 +112,7 @@ public class PlaywrightTestAttribute : TestAttribute, IApplyToContext, IApplyToT try { innerCommand.Execute(context); - if (context.CurrentResult.ResultState == ResultState.Success) + if (context.CurrentResult.ResultState == ResultState.Success || context.CurrentResult.ResultState == ResultState.Skipped || context.CurrentResult.ResultState == ResultState.Ignored) { isPassed = true; break; @@ -205,4 +196,71 @@ public class PlaywrightTestAttribute : TestAttribute, IApplyToContext, IApplyToT _unobservedTaskExceptions.Add(e.Exception); } } + + public class TimeoutCommand : BeforeAndAfterTestCommand + { + private readonly int _timeout; + + internal TimeoutCommand(TestCommand innerCommand, int timeout) : base(innerCommand) + { + _timeout = timeout; + } + + public override TestResult Execute(TestExecutionContext context) + { + try + { + using (new TestExecutionContext.IsolatedContext()) + { + var testExecution = Task.Run(() => innerCommand.Execute(TestExecutionContext.CurrentContext)); + var timedOut = Task.WaitAny([testExecution], _timeout) == -1; + + if (timedOut) + { + context.CurrentResult.SetResult( + ResultState.Failure, + $"Test exceeded Timeout value of {_timeout}ms"); + // When the timeout is reached the TearDown methods are not called. This is a best-effort + // attempt to call them and close the browser / http server. + foreach (var tearDown in GetHackyTearDownMethods(context)) + { + tearDown(); + } + } + else + { + context.CurrentResult = testExecution.GetAwaiter().GetResult(); + } + } + } + catch (Exception exception) + { + context.CurrentResult.RecordException(exception, FailureSite.Test); + } + + return context.CurrentResult; + } + + private Action[] GetHackyTearDownMethods(TestExecutionContext context) + { + var methods = new List(); + foreach (var method in new string[] { "WorkerTeardown", "BrowserTearDown" }) + { + var methodFun = context.CurrentTest.Method.MethodInfo.DeclaringType + .GetMethod(method, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic); + if (methodFun != null) + { + methods.Add(() => + { + var maybeTask = methodFun.Invoke(context.TestObject, null); + if (maybeTask is Task task) + { + task.GetAwaiter().GetResult(); + } + }); + } + } + return methods.ToArray(); + } + } } diff --git a/src/Playwright.Tests/BaseTests/HttpService.cs b/src/Playwright.Tests/BaseTests/HttpService.cs index 7daf328e..e5b95d16 100644 --- a/src/Playwright.Tests/BaseTests/HttpService.cs +++ b/src/Playwright.Tests/BaseTests/HttpService.cs @@ -39,8 +39,8 @@ public class HttpService : IWorkerService var assetDir = Path.Combine(TestUtils.FindParentDirectory("Playwright.Tests"), "assets"); var http = new HttpService { - Server = SimpleServer.Create(8907 + workerIndex * 2, assetDir), - HttpsServer = SimpleServer.CreateHttps(8907 + workerIndex * 2 + 1, assetDir) + Server = SimpleServer.Create(8907 + workerIndex * 4, assetDir), + HttpsServer = SimpleServer.CreateHttps(8907 + workerIndex * 4 + 1, assetDir) }; await Task.WhenAll(http.Server.StartAsync(TestContext.CurrentContext.CancellationToken), http.HttpsServer.StartAsync(TestContext.CurrentContext.CancellationToken)); return http; diff --git a/src/Playwright.Tests/BrowserTypeConnectTests.cs b/src/Playwright.Tests/BrowserTypeConnectTests.cs index 7a5809fb..466857b5 100644 --- a/src/Playwright.Tests/BrowserTypeConnectTests.cs +++ b/src/Playwright.Tests/BrowserTypeConnectTests.cs @@ -451,8 +451,7 @@ public class BrowserTypeConnectTests : PlaywrightTestEx } } - [PlaywrightTest("browsertype-connect.spec.ts", "should upload large file")] - [CancelAfter(TestConstants.SlowTestTimeout)] + [PlaywrightTest("browsertype-connect.spec.ts", "should upload large file", TestConstants.SlowTestTimeout)] public async Task ShouldUploadLargeFile() { var browser = await BrowserType.ConnectAsync(_remoteServer.WSEndpoint); diff --git a/src/Playwright.Tests/ClientCertficatesTests.cs b/src/Playwright.Tests/ClientCertficatesTests.cs index a55dd04e..52b5a8d1 100644 --- a/src/Playwright.Tests/ClientCertficatesTests.cs +++ b/src/Playwright.Tests/ClientCertficatesTests.cs @@ -103,11 +103,6 @@ public class ClientCertificatesTests : BrowserTestEx { Assert.Ignore("WebKit on macOS doesn't proxy localhost requests"); } - if (TestConstants.IsChromium && TestConstants.IsWindows) - { - // TODO: Remove after https://github.com/microsoft/playwright/issues/17252 is fixed. - Assert.Ignore("Chromium on Windows doesn't proxy localhost requests"); - } } [PlaywrightTest("", "")] diff --git a/src/Playwright.Tests/PageSetInputFilesTests.cs b/src/Playwright.Tests/PageSetInputFilesTests.cs index f5795e0d..6a76dfd3 100644 --- a/src/Playwright.Tests/PageSetInputFilesTests.cs +++ b/src/Playwright.Tests/PageSetInputFilesTests.cs @@ -352,8 +352,7 @@ public class PageSetInputFilesTests : PageTestEx Assert.True(fileChooser.IsMultiple); } - [PlaywrightTest("page-set-input-files.spec.ts", "should upload large file")] - [CancelAfter(TestConstants.SlowTestTimeout)] + [PlaywrightTest("page-set-input-files.spec.ts", "should upload large file", TestConstants.SlowTestTimeout)] public async Task ShouldUploadLargeFile() { await Page.GotoAsync(Server.Prefix + "/input/fileupload.html"); diff --git a/src/Playwright.Tests/PageWaitForNavigationTests.cs b/src/Playwright.Tests/PageWaitForNavigationTests.cs index 74e8c0bd..ce6aef57 100644 --- a/src/Playwright.Tests/PageWaitForNavigationTests.cs +++ b/src/Playwright.Tests/PageWaitForNavigationTests.cs @@ -306,8 +306,7 @@ public class PageWaitForNavigationTests : PageTestEx StringAssert.Contains("/frames/one-frame.html", Page.Url); } - [PlaywrightTest] - [CancelAfter(TestConstants.SlowTestTimeout)] + [PlaywrightTest(TestConstants.SlowTestTimeout)] public async Task ShouldHaveADefaultTimeout() { await Page.GotoAsync(Server.Prefix + "/frames/one-frame.html"); diff --git a/src/Playwright.Tests/Playwright.Tests.csproj b/src/Playwright.Tests/Playwright.Tests.csproj index 16be4fba..e606aa57 100644 --- a/src/Playwright.Tests/Playwright.Tests.csproj +++ b/src/Playwright.Tests/Playwright.Tests.csproj @@ -26,9 +26,9 @@ runtime; build; native; contentfiles; analyzers; buildtransitive all - - - + + +