Skip to content

Better BuildServerTestFixtureBase dispose timeouts #29544

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand Down Expand Up @@ -225,7 +225,7 @@ public async Task Build_WithWhiteSpaceInPipeName_BuildsSuccessfully()
finally
{
// Shutdown the server
fixture.Dispose();
await fixture.DisposeAsync();
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
Expand All @@ -11,7 +11,7 @@ namespace Microsoft.AspNetCore.Razor.Design.IntegrationTests
/// Note that this fixture will always initialize a server of the current version since it
/// invokes the ServerConnection API from the referenced rzc.
/// </summary>
public class BuildServerTestFixture : BuildServerTestFixtureBase, IDisposable
public class BuildServerTestFixture : BuildServerTestFixtureBase
{
public BuildServerTestFixture() : this(Guid.NewGuid().ToString())
{
Expand Down
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.InteropServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Razor.Tools;
using Microsoft.CodeAnalysis;
using Moq;
using Xunit;

namespace Microsoft.AspNetCore.Razor.Design.IntegrationTests
{
public abstract class BuildServerTestFixtureBase : IDisposable
public abstract class BuildServerTestFixtureBase : IAsyncLifetime
{
private static readonly TimeSpan _defaultShutdownTimeout = TimeSpan.FromSeconds(120);

Expand All @@ -23,19 +25,18 @@ protected BuildServerTestFixtureBase(string pipeName)

public string PipeName { get; }

public void Dispose()
public Task InitializeAsync()
{
return Task.CompletedTask;
}

public async Task DisposeAsync()
{
// Shutdown the build server.
using (var cts = new CancellationTokenSource(_defaultShutdownTimeout))
{
var writer = new StringWriter();

cts.Token.Register(() =>
{
var output = writer.ToString();
throw new TimeoutException($"Shutting down the build server at pipe {PipeName} took longer than expected.{Environment.NewLine}Output: {output}.");
});

var application = new Application(cts.Token, Mock.Of<ExtensionAssemblyLoader>(), Mock.Of<ExtensionDependencyChecker>(), (path, properties) => Mock.Of<PortableExecutableReference>(), writer, writer);

var args = new List<string>
Expand All @@ -52,7 +53,21 @@ public void Dispose()
args.Add("-w");
}

var exitCode = application.Execute(args.ToArray());
var executeTask = Task.Run(() => application.Execute(args.ToArray()));

if (executeTask == await Task.WhenAny(executeTask, Task.Delay(Timeout.Infinite, cts.Token)))
{
// Complete the Task.Delay
cts.Cancel();
}
else
{
var output = writer.ToString();
throw new TimeoutException($"Shutting down the build server at pipe {PipeName} took longer than expected.{Environment.NewLine}Output: {output}.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if instead the code should just output a warning e.g. using an ITestOutputHelper. Unfortunately that would require changing all uses of derived fixtures. Thoughts i.e. is this case actually an error worth crashing the test host about❔

Copy link
Member Author

Choose a reason for hiding this comment

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

After this change, the tests fail rather than crash the testhost. I don't know if a warning would be better, but it sounds like this isn't going to be around for much longer. This is a simple fix that seems to me to be strictly better than what we have now.

}

var exitCode = await executeTask;

if (exitCode != 0)
{
var output = writer.ToString();
Expand Down
2 changes: 1 addition & 1 deletion src/Shared/CertificateGeneration/CertificateManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -942,7 +942,7 @@ public void TrustCertificateStart(string certificate)
}

[Event(30, Level = EventLevel.Verbose)]
public void TrustCertificateEnd() =>WriteEvent(30, "Finished trusting the certificate.");
public void TrustCertificateEnd() => WriteEvent(30, "Finished trusting the certificate.");

[Event(31, Level = EventLevel.Error)]
public void TrustCertificateError(string ex)
Expand Down