OSOE-935: Upgrade to xUnit 3 and handling test cancellations gracefully #432

Piedone · 2024-12-22T01:16:48Z

Test cancellation can be tried when running the exe of a test: run it from the CLI, wait like 10 seconds, hit Ctrl+C (only once), and you should see "Test execution was cancelled. Shutting down the test execution session." at one point (only once the tests actually started).

To be added to the release notes:

Upgraded xUnit to v3 with breaking changes

This version of the UI Testing Toolbox uses v3 of xUnit, which brings many updates. However, it's also a breaking version, requiring you to adapt your test projects, see the official guide.

Migrating UI test projects consuming the UI Testing Toolbox should be simpler, though, with you requiring to do roughly the following steps:

Update the xunit.runner.visualstudio package reference to latest (currently 3.0.0).
Convert test projects to executables, see docs (this will most possibly only need you to add <OutputType>Exe</OutputType> to the first PropertyGroup in the test project's csproj). These are all the projects with xunit.runner.visualstudio references.
In all test projects, change xunit package references to xunit.v3 with the latest version (currently 1.0.0). If a test project lacks such a reference then add it (currently <PackageReference Include="xunit.v3" Version="1.0.0" />). Only test projects should reference xunit.
Also update the Microsoft.NET.Test.Sdk references in the test projects to latest (currently 17.12.0) too while we're at it, or add it if it's missing (currently <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.12.0" />).
ITestOutputHelper is now in the Xunit namespace instead of Xunit.Abstractions. Fix the namespace imports in UITestBase classes first and then anywhere else; they'll show up as build errors.
Clean up unused namespace imports. In Visual Studio, you can do this by right-clicking on the solution -> Analyze and Code Cleanup -> Run Code Cleanup (Profile 1).
Run the tests to check if everything is still OK. If tests don't show up in the Visual Studio Test Explorer, or show up but don't start if you try to run them, check out the Tests pane of the Output Window for clues. Confirm that the Web project doesn't contain any xUnit references (even transitively, like by accidentally referencing UI test projects) but all test projects reference unit.v3. You might need to do a recursive git clean to be sure to start with a clean slate.
Verify in CI that the tests are actually discovered and all run there too (the CI build being green is not enough, it might miss no tests being discovered).

# Conflicts: # Lombiq.Tests.UI/CompatibilitySuppressions.xml

dministro · 2025-01-16T18:32:36Z

Lombiq.Tests.UI.Samples/Tests/RemoteTests.cs

+
+// While there may be some common code between local and remote tests, they cannot be the same, and the common code must
+// be in a project independent of the web app (which is not the case here for the sake of simplicity):
+// - In local tests, you have a much lower-level access to the app: E.g., you can access the Orchard Core log, use


Did you mean "higher-level"?

Suggested change

// - In local tests, you have a much lower-level access to the app: E.g., you can access the Orchard Core log, use

// - In local tests, you have much higher-level access to the app: E.g., you can access the Orchard Core log, use

I did mean lower level: the app runs in the same process, so you can communicate with its container and types directly, whereas in prod, all you have is the public UI.

dministro · 2025-01-16T18:34:35Z

Lombiq.Tests.UI/BasicOrchardFeaturesTesting/WorkflowsFeatureTestingUITestContextExtensions.cs

@@ -48,6 +48,8 @@ public static Task TestWorkflowsAsync(this UITestContext context) =>
                    By.XPath("//div[@class = 'jtk-endpoint jtk-endpoint-anchor jtk-draggable jtk-droppable']"), // #spell-check-ignore-line
                    By.XPath(taskXPath));

+                context.WaitElementToNotChange(By.ClassName("jtk-connector")); // #spell-check-ignore-line


Why this change is necessary?

This waits for the connector to settle after the drag and drop. The test was flaky without this, since the connection might not be registered and thus saved before it.

dministro · 2025-01-16T18:53:11Z

Lombiq.Tests.UI/Models/UITestManifest.cs

-    public ITest XunitTest { get; }
-    public string Name => XunitTest?.DisplayName;
+    public ITest XunitTest => TestContext.Current.Test;
+    public string Name => XunitTest?.TestDisplayName;


Can TestContext.Current.Test null here?

Not, actually.

dministro · 2025-01-16T18:56:10Z

Lombiq.Tests.UI/Models/UITestManifest.cs

-    public ITest XunitTest { get; }
-    public string Name => XunitTest?.DisplayName;
+    public ITest XunitTest => TestContext.Current.Test;
+    public string Name => XunitTest?.TestDisplayName;
    public Func<UITestContext, Task> TestAsync { get; set; }


This can be a constructor parameter.

dministro · 2025-01-16T18:56:43Z

Lombiq.Tests.UI/OrchardCoreUITestBase.cs

@@ -369,7 +369,7 @@ protected virtual async Task ExecuteTestAsync(
        Func<UITestContext, Task<Uri>> setupOperation,
        Func<OrchardCoreUITestExecutorConfiguration, Task> changeConfigurationAsync)
    {
-        var testManifest = new UITestManifest(_testOutputHelper) { TestAsync = testAsync };
+        var testManifest = new UITestManifest { TestAsync = testAsync };


See my comment for UITestManifest.TestAsync.

dministro · 2025-01-16T18:57:18Z

Lombiq.Tests.UI/RemoteUITestBase.cs

@@ -61,7 +61,7 @@ async Task BaseUriVisitingTest(UITestContext context)
            await testAsync(context);
        }

-        var testManifest = new UITestManifest(_testOutputHelper) { TestAsync = BaseUriVisitingTest };
+        var testManifest = new UITestManifest { TestAsync = BaseUriVisitingTest };


See my comment for UITestManifest.TestAsync.

dministro · 2025-01-16T21:37:35Z

Lombiq.Tests.UI/Services/UITestExecutionSession.cs

@@ -61,6 +61,12 @@ public UITestExecutionSession(

    public async Task<bool> ExecuteAsync(int retryCount, string dumpRootPath)
    {
+        _configuration.TestCancellationToken.Register(() =>


I'm not sure, but I think System.Threading.CancellationTokenRegistration returned by CancellationToken.Register needs to be disposed.

Is this really necessary?

Testing _configuration.TestCancellationToken.IsCancellationRequested also wellcome before starting the whole ceremony.

It indeed needs to be disposed, so I added that.

I don't really understand the rest of your comment: xUnit 3 supports cancellations. If we want the tests to clean up after themselves when such a cancellation happens, hooking into it is necessary. Running ShutdownAsync() will do the clean-up. Thus after this, a cancelled test won't leave ChromeDriver processes or browser windows open.

dministro · 2025-01-16T21:45:19Z

Lombiq.Tests.UI/Services/UITestExecutionSession.cs

+            if (timeoutTask.IsCompleted)
+            {
+                // If the EnterInteractiveModeAsync() extension method has been used, then timeout should be ignored to
+                // make the debugging experience smoother. Note that EnterInteractiveModeAsync() should never be used in
+                // committed tests.
+                if (!ShortcutsUITestContextExtensions.InteractiveModeHasBeenUsed)
+                {
+                    throw new TimeoutException($"The time allotted for the test ({timeout}) was exceeded.");
+                }
+
+                await testTask;
+            }
+
+            // Since the timeout task is not yet completed but the Task.WhenAny has finished, the test task is done in
+            // some way. So it's safe to await it here. It's also necessary to cleanly propagate any exceptions that may
+            // have been thrown inside it.
+            await testTask;


In case when a cancellation is requested on the _configuration.TestCancellationToken, the timeoutTask.IsCompleted will be true.

dministro · 2025-01-16T21:49:03Z

Lombiq.Tests.UI/Services/UITestExecutor.cs

@@ -75,11 +60,6 @@ private static async Task ExecuteOrchardCoreTestInnerAsync(
        {


The _configuration.TestCancellationToken.IsCancellationRequested can also stop the loop.

This reverts commit 85972f5.

…ldWait fails

… tests

…body

…reliable

Piedone · 2025-01-23T04:14:25Z

Lombiq.Tests.UI.Samples/Tests/InteractiveModeTests.cs

-    // This test checks if interactive mode works by opening it in one thread, and then clicking it away in a different
-    // thread. Two threads are necessary because interactive mode stops test execution on its current thread, so we
-    // wouldn't be able to end it from within a test.
-    [Fact]
-    public Task EnteringInteractiveModeShouldWait() =>
-        ExecuteTestAfterSetupAsync(
-            async context =>
-            {
-                var currentUrl = context.Driver.Url;
-
-                await Task.WhenAll(
-                    context.SwitchToInteractiveAsync(),
-                    Task.Run(
-                        async () =>
-                        {
-                            try
-                            {
-                                ReliabilityHelper.DoWithRetriesOrFail(
-                                    () => context.Driver.WindowHandles.Count > 1,
-                                    TimeSpan.FromSeconds(15));
-
-                                context.SwitchToLastWindow();
-
-                                await context.ClickReliablyOnAsync(By.ClassName("interactive__continue"));
-                            }
-                            catch (Exception ex)
-                            {
-                                _testOutputHelper.WriteLineTimestampedAndDebug(
-                                    "Interactive mode wasn't canceled properly due to the following exception. Canceling the test. {0}",
-                                    ex);
-
-                                // The other thread will wait indefinitely if the button wasn't clicked in the end. So,
-                                // failing the test then.
-                                TestContext.Current.CancelCurrentTest();
-                            }
-                        },
-                        context.Configuration.TestCancellationToken));
-
-                // Ensure that the info tab is closed and the control handed back to the last tab.
-                context.Driver.Url.ShouldBe(currentUrl);
-            });


I've spent half a day trying to make this reliable, i.e. not hang randomly (that hung the tests too), and be reliable with its interactions. I couldn't get there, it was always flaky. I don't see any point in trying harder, we don't really need this test.

This reverts commit 5c2b55c.

Piedone added 2 commits December 21, 2024 23:33

Migrating to xUnit v3

345b95a

Passing TestContext.Current.CancellationToken

de63424

github-actions bot changed the title ~~Upgrade to xUnit 3 and handling test cancellations gracefully~~ OSOE-935: Upgrade to xUnit 3 and handling test cancellations gracefully Dec 22, 2024

Piedone added 20 commits December 22, 2024 02:26

Implementing test cancellation

cb34796

Updating Lombiq.Tests reference

4f944f6

Shutdown messages

9d93de4

Suppressing breaking changes

41af070

Using the central cancellation token

0e883bf

Merge remote-tracking branch 'origin/dev' into issue/OSOE-935

b7c1651

Merge remote-tracking branch 'origin/dev' into issue/OSOE-935

fc9af11

Merge remote-tracking branch 'origin/dev' into issue/OSOE-935

73d0935

# Conflicts: # Lombiq.Tests.UI/CompatibilitySuppressions.xml

Removing leftover Xunit.Abstractions references

97363e2

Referencing xunit.v3.extensibility.core instead of xunit.v3.core

adac087

Updating Lombiq NuGet references

713d4f7

Making the Workflow test of BasicOrchardFeaturesTesting more reliable

3ca6fc6

Moving test timeout management to UITestExecutionSession

6a0fac4

Spelling

386d738

Removing debug code

0b554d7

Merge remote-tracking branch 'origin/dev' into issue/OSOE-935

2db3a83

Making EnteringInteractiveModeShouldWait more reliable

7e2ce01

More docs on remote tests

9f535c0

Spelling

9106efe

Fixing that TestRunTimeoutShouldThrowAsync always created a test dump

474a347

dministro requested changes Jan 16, 2025

View reviewed changes

Piedone added 6 commits January 17, 2025 00:01

Disabling HTML validation as a test

ff2fbb5

Merge remote-tracking branch 'origin/dev' into issue/OSOE-935

9095af6

Removing unneeded null check

7dbae5a

Simplification

9711cee

Disposing TestCancellationToken registration

c322704

Updating CompatibilitySuppressions

20c0fa9

Piedone added 27 commits January 22, 2025 00:55

Preventing EnteringInteractiveModeShouldWait from hanging

e2f5cf2

Reverting some debug code

d5b2825

Trying recursive chmod

803e046

Fixing debug code

4672c58

Logging the exception too during EnteringInteractiveModeShouldWait

19d323e

Fixing chmod

115d806

sudo chmod

9da6204

More chmod

e1d7b3c

Removing chmod override

b473b8d

Not running clean-up on GitHub

401984f

Revert "Temporarily removing Validate NuGet Publish"

5c08ea3

This reverts commit 85972f5.

Reverting debug change

21ebbcc

Spelling

8e4d594

Spelling

09f4274

Not canceling the whole test execution if EnteringInteractiveModeShou…

49432ab

…ldWait fails

Suppressing breaking changes

eb9ca06

Passing cancellation tokens to SwitchToInteractiveAsync

7ee2479

And more

4642b4a

Fixing that the setup of CustomAdminPrefixTestCases could break other…

ba3c10f

… tests

Spelling

fc3f7e2

Making EnteringInteractiveModeShouldWait safer

a47b79b

Only close interactive mode window if it's still open

d6c5098

Trying to make EnteringInteractiveModeShouldWait safer

aa8c3f9

PageNavigationState won't throw if the response doesn't have an html …

13efcfb

…body

Safer interactive mode exit

3fd867a

More convenient interactive mode demo

1bcb543

Removing EnteringInteractiveModeShouldWait that's impossible to make …

29aa2fa

…reliable

Piedone commented Jan 23, 2025

View reviewed changes

Piedone added 2 commits January 23, 2025 21:48

Merge remote-tracking branch 'origin/dev' into issue/OSOE-935

c680af4

Revert "Temporarily disabling SendingTestEmailShouldWork too"

2398546

This reverts commit 5c2b55c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSOE-935: Upgrade to xUnit 3 and handling test cancellations gracefully #432

OSOE-935: Upgrade to xUnit 3 and handling test cancellations gracefully #432

Piedone commented Dec 22, 2024 •

edited

Loading

dministro Jan 16, 2025

Piedone Jan 20, 2025

dministro Jan 16, 2025

Piedone Jan 20, 2025

dministro Jan 16, 2025

Piedone Jan 20, 2025

dministro Jan 16, 2025

dministro Jan 16, 2025

dministro Jan 16, 2025

dministro Jan 16, 2025 •

edited

Loading

Piedone Jan 20, 2025

dministro Jan 16, 2025

dministro Jan 16, 2025

Piedone Jan 23, 2025

	// - In local tests, you have a much lower-level access to the app: E.g., you can access the Orchard Core log, use
	// - In local tests, you have much higher-level access to the app: E.g., you can access the Orchard Core log, use

		@@ -75,11 +60,6 @@ private static async Task ExecuteOrchardCoreTestInnerAsync(
		{

OSOE-935: Upgrade to xUnit 3 and handling test cancellations gracefully #432

Are you sure you want to change the base?

OSOE-935: Upgrade to xUnit 3 and handling test cancellations gracefully #432

Conversation

Piedone commented Dec 22, 2024 • edited Loading

Upgraded xUnit to v3 with breaking changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dministro Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Piedone commented Dec 22, 2024 •

edited

Loading

dministro Jan 16, 2025 •

edited

Loading