Skip to content
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.

Sign up for GitHub

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

Disable test parallelization in installer tests #58604

Closed
wants to merge 1 commit into from
Closed

Disable test parallelization in installer tests #58604

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions ...staller/tests/Microsoft.NET.HostModel.Tests/AppHost.Bundle.Tests/BundledAppWithSubDirs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Threading;
using System.Diagnostics;
using BundleTests.Helpers;
using Microsoft.DotNet.Cli.Build.Framework;
using Microsoft.DotNet.CoreSetup.Test;
using Microsoft.NET.HostModel.Bundle;
using Xunit;

[assembly: CollectionBehavior(DisableTestParallelization = true)]

namespace AppHost.Bundle.Tests
{
public class BundledAppWithSubDirs : BundleTestBase, IClassFixture<BundledAppWithSubDirs.SharedTestState>
Expand Down
2 changes: 2 additions & 0 deletions .../tests/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.Bundle.Tests/BundleAndRun.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
using BundleTests.Helpers;
using System.Runtime.InteropServices;

[assembly: CollectionBehavior(DisableTestParallelization = true)]

namespace Microsoft.NET.HostModel.Tests
{
public class BundleAndRun : IClassFixture<BundleAndRun.SharedTestState>
Expand Down
14 changes: 14 additions & 0 deletions src/installer/tests/TestUtils/TestProjectFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Generic;
using System.IO;
using System.Threading;

namespace Microsoft.DotNet.CoreSetup.Test
{
Expand Down Expand Up @@ -75,10 +76,23 @@ public void Dispose()
}
}

static readonly object s_lock = new object();

private TestProject CopyTestProject(TestProject sourceTestProject)
{
// This can race with the filesystem. If we're running this code from two threads, fail fast
Copy link
Member

Choose a reason for hiding this comment

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

Even with test parallelization disabled, does xunit guarantee it'll always run tests on the same single thread? I'd have expected it'd be valid for it to run tests on different threads, just serialized.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't guarantee it as far as I can tell, but it happens to work for now.

I'm not particularly pleased with this solution, but it's the closest thing I can find to a workaround that will scream if something goes wrong.

Long term plan needs to be to make these tests thread safe

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephentoub Any suggestions? If this starts failing I can remove the lock

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this fails when xunit is allowed to run concurrently and whether the tests really touch the same files.

Copy link
Member

Choose a reason for hiding this comment

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

I am concerned that we are not dealing with concurrent use of the same file issue. If we do not realy have that, then change may have effect, but only because it makes tests to run slower or smth and would only reduce failures, not eliminate them.

Copy link
Member

Choose a reason for hiding this comment

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

What I suspect is the root cause is that in rare cases the file system has not finished writing the executable and we already try executing it. I do not know though how to prove this or how to prevent this from happening.

Copy link
Member

Choose a reason for hiding this comment

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

What I find suspicious is that bundle tests are not the only tests which write executables and then try to run them immediately after. Lot of the hosting tests do this - they either build the app and immediately run it, or in some cases we take a built app, patch the apphost and then run it (patching it to enable some test features in the product).

The only main difference between the hosting tests and the bundle tests is the size of the executable. Hosting tests use the standard apphost (150KB), while the bundle tests create large single-file executables (60MB+).

So it is definitely possible that the file system didn't really finish writing it yet. Or there's some other thing (like AV) taking longer due to the size of the file. It's also interesting that pretty much all the failures are on Linux.

bool lockTaken = false;
object lockObj = s_lock;
Monitor.TryEnter(lockObj, 0, ref lockTaken);
if (!lockTaken)
{
throw new InvalidOperationException("Lock is held, test should not be executed in parallel");
}

EnsureDirectoryBuildFiles(TestArtifact.TestArtifactsPath);
return sourceTestProject.Copy();

// Don't release the lock. There should be only one thread running these tests
}

private void EnsureDirectoryBuildFiles(string testArtifactDirectory)
Expand Down