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.

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

DisableAppDomain switch is not fully respected for Discovery #331

Closed
nohwnd opened this issue Jul 26, 2022 · 6 comments
Closed

DisableAppDomain switch is not fully respected for Discovery #331

nohwnd opened this issue Jul 26, 2022 · 6 comments

Comments

@nohwnd
Copy link
Contributor

nohwnd commented Jul 26, 2022

I am testing with Roslyn like solution, to see how much gain we would get by passing DisableAppDomain switch by default in discovery to avoid the slowness of RealProxy xunit/xunit#2323, because we run in parallel by default now, and isolate on process level.

DisableAppDomain vstest option switch was implemented in xunit/xunit#944, but I don't think it is fully propagated for discovery. When I debug the testhost process I can see there are still 4 appdomains. And I can see a lot of time is spent remoting between them.

I am also unsure if CollectSourceInformation is propagated fully, because that I see dia session being created, in a separate appdomain, but maybe it is created but later not used (honoring the option is implemented in xunit/xunit#1352).

When I crudely patch v2 xunit and xunit.runner.visualstudio to always use AppDomainManager_NoAppDomain, and NullSourceInformationProvider, the discovery time goes from ~8 seconds down to ~2 for the biggest projects with ~25k tests each:

Before, ~8s per project:

[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.3+1b45f5407b (64-bit Desktop .NET 4.0.30319.42000)
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.3+1b45f5407b (64-bit Desktop .NET 4.0.30319.42000)
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.37]   Discovering: Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.37]   Discovering: Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:07.47]   Discovered:  Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:08.01]   Discovered:  Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472
[DISCOVERY.COMPLETE] aborted? False, tests count: 54628, discovered count: 54628
Last chunk:
Fully discovered:
        S:\t\fake-roslyn\FakeRoslynXunit\Generated\Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472\bin\Debug\net472\Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472.dll
        S:\t\fake-roslyn\FakeRoslynXunit\Generated\Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472\bin\Debug\net472\Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472.dll
Partially discovered:
        <empty>
Skipped discovery:
        <empty>
Not discovered:
        <empty>
Discovery done in 14803 ms
Discovery: 14803 ms, Run: 0 ms, Total: 14803 ms

After ~2s per project:

[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.6-preview.4+b0b4704a7c (64-bit Desktop .NET 4.0.30319.42000)
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.6-preview.4+b0b4704a7c (64-bit Desktop .NET 4.0.30319.42000)
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.25]   Discovering: Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:00.24]   Discovering: Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:01.85]   Discovered:  Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472
[DISCOVERY.INFORMATIONAL] [xUnit.net 00:00:01.96]   Discovered:  Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472
[DISCOVERY.COMPLETE] aborted? False, tests count: 54628, discovered count: 54628
Last chunk:
Fully discovered:
        S:\t\fake-roslyn\FakeRoslynXunit\Generated\Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472\bin\Debug\net472\Microsoft.CodeAnalysis.CSharp.EditorFeatures.UnitTests-net472.dll
        S:\t\fake-roslyn\FakeRoslynXunit\Generated\Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472\bin\Debug\net472\Microsoft.CodeAnalysis.CSharp.EditorFeatures2.UnitTests-net472.dll
Partially discovered:
        <empty>
Skipped discovery:
        <empty>
Not discovered:
        <empty>
Discovery done in 8681 ms
Discovery: 8681 ms, Run: 0 ms, Total: 8681 ms
  • Did you create a simple repro for the problem?

I don't have a simple repro for the fix, but .NET does not use appdomains (and is overall faster), so creating a solution with 25k tests, and switching between net48, and net6.0 target frameworks shows significantly different times it takes to discover the project under VS.

<!-- file combi.csproj -->
<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net472</TargetFramework> <!-- switch to net6.0 and rebuild  -->
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <LangVersion>Latest</LangVersion>

    <IsPackable>false</IsPackable>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.1.0" />
    <PackageReference Include="Xunit.Combinatorial" Version="1.5.7-beta" />
    <PackageReference Include="xunit" Version="2.4.1" />
    <PackageReference Include="xunit.runner.visualstudio" Version="2.4.3">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
    <PackageReference Include="coverlet.collector" Version="3.1.2">
      <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
      <PrivateAssets>all</PrivateAssets>
    </PackageReference>
  </ItemGroup>

</Project>
// file UnitTest1.cs
namespace combi;

public class UnitTest1
{
    [Theory, CombinatorialData]
    public void CombinatorialRandomValuesCount([CombinatorialRange(0, 25000)] int p1)
    {
        Assert.InRange(p1, 0, int.MaxValue);
    }
}
// file Usings.cs
global using Xunit;

Would you take a fix for this for v2?

@nohwnd
Copy link
Contributor Author

nohwnd commented Aug 3, 2022

@bradwilson not trying to be pushy, but this would really help us speed up things under VS. Is it okay if I PR fixes for it?

@bradwilson bradwilson transferred this issue from xunit/xunit Aug 3, 2022
@bradwilson
Copy link
Member

It's up to @clairernovotny to decide which fixes to take and when releases are made.

@nohwnd
Copy link
Contributor Author

nohwnd commented Aug 19, 2022

@clairernovotny polite bump on this one.

@bradwilson
Copy link
Member

@nohwnd FYI @clairernovotny has stepped away from this project, but if you are able to provide a PR, I can put it into the 2.5.0 release (I anticipate a final build happening in the next few weeks).

@nohwnd
Copy link
Contributor Author

nohwnd commented May 23, 2023

I am not sure if I will be able to produce and test it in the next 2 weeks, but I take this as a GO and will look into it when I have some time.

@bradwilson
Copy link
Member

Coming to a 2.5.0 near you, soon...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants