microsoft / testfx

MSTest framework and adapter
MIT License
714 stars 253 forks source link

In-assembly parallel execution #142

Closed pvlakshm closed 6 years ago

pvlakshm commented 7 years ago

Description

Enable tests runs to complete faster by allowing tests within a single assembly to execute in parallel.

Please see RFC here: 004-In-Assembly-Parallel-Execution

AbhitejJohn commented 7 years ago

Here is the RFC for this. Feedback is welcome.

andersforsgren commented 7 years ago

Should parallel test execution not (at least optionally) be possible for non parallel-ready code? A container with 1000 tests that can't run in parallel can be divided in chunks and run sequentially in N appdomains/processes.

rajbos commented 7 years ago

We have been using ‪ https://github.com/sscobici/ParallelTestRunner for this for a while now. Can split on Class level, Method level and group tests via annotations. Would be very nice to have this build in, so there is no longer a need to run an executable to split the tests, run them and then summarize the testresults.

AdamDotNet commented 7 years ago

I'm all for letting tests run in parallel, especially since it doesn't seem to work right now in MSTestV2, at least in Visual Studio 2015. The RFC linked mentions VSTest is capable of allowing all test providers to do test parallelization by spawning multiple processes.

I know this feature is for in assembly parallelization, but for my sake, can someone confirm to me that regular old VSTest multiprocess parallelization isn't meant to work right now in MSTestV2? I would appreciate the clarity. Thank you!

AbhitejJohn commented 7 years ago

@AdamDotNet : The container level parallelization for vstest works irrespective of the Framework(mstestv2/mstest/xunit/nunit). If your solution has multiple test projects, it should spawn a host process for each test container and run them in parallel when enabled just like this blog details. This only works if there is more than one test container. You aren't seeing that happen in VS 2015?

AdamDotNet commented 7 years ago

@AbhitejJohn Thanks for the information. I never realized that multiprocess parallelism was dependent on having multiple projects. I just observed it working in a sample project with two test projects.

Back to this feature of in assembly parallelism, if I'm reading the RFC right, you can mark an assembly with how you want it to handle parallelism in that assembly. I'm imagining in my solution, it would be generally acceptable to mark the assembly as parallel by class by default, but it would be nice if certain classes could run faster by running the test methods in a particular class in parallel.

So, will it be possible to default an assembly to class level parallelization, but you can override that behavior by placing attributes on a specific class to enable method parallelization for that test class? Will this work with only one test project?

To illustrate, I have one test project with the desire for the following behavior:

AbhitejJohn commented 7 years ago

@AdamDotNet : That's a great point. This is essentially a mix and match of multiple modes in the same assembly. Our thought process was that integration/End-to-End tests would actually be split into a different assembly from unit tests, whose test mode can then be configured accordingly.

That said, for the scenario described above, as long as the fast running tests within a test class do not interfere with each other, the mode for the assembly can be made method level as a whole. That ensures that all methods irrespective of what class they are under are run in parallel.

However this does look like a valid ask and could surface more when we add in custom modes. The only choice that the RFC provides today is to opt-out of the default mode for an assembly. We hope that these scenarios however are relatively fewer in number and come under a more advanced set of capabilities that we can add in the future. Do let us know otherwise.

JoshCollinsMSFT commented 7 years ago

This is something that is incredibly frustrating to my team. This seems like a regression in behavior from the parallelization control we got from the testsettings file. In fact, my team is still currently are using a testsettings file to get that level of parallelization of our tests because one of our test categories takes around 3 hours to complete without it, and only 20 minutes to complete with it (with a setting of parallelTestCount="18").

Unfortunately we're banging our heads against this problem right now because without using a runsettings file we can't easily use test parameters, and using a runsettings file we can't get parallel execution.

The sooner we can get support for this in MSTest while using a runsettings file the better.

AbhitejJohn commented 7 years ago

Thanks for letting us know @JoshCollinsMSFT . We are in the phase of collecting feedback on the RFC that Pratap posted above. Do let us know if that meets your requirements. We plan to start off on this in the weeks to come.

JoshCollinsMSFT commented 7 years ago

@AbhitejJohn How do we provide feedback to the RFC?

AbhitejJohn commented 7 years ago

@JoshCollinsMSFT : you can do so here :)

JoshCollinsMSFT commented 7 years ago

The RFC looks good. The only comments I have are that I hope the runsettings file will allow you to set both TestParallelization as well as TestParallelizationLevel.

AbhitejJohn commented 7 years ago

@JoshCollinsMSFT : We deliberately left that out. In our thought process TestParallelizationLevel is environment dependent and not-really tied to a test container. Users would want to change this globally and want that to take precedence over what is specified in an assembly based on what environment they are run on. TestParallelization on the other hand is specific to a test container. Users would explicitly add that into a container, as opposed to letting the default kick in, because that specific container functions best in that mode. One would not also want a global setting to override the assembly specification in this case. Adding in a global default which unlike TestParallelizationLevel does not take precedence over the assembly specification would cause more confusion. Hence we left that out.

JoshCollinsMSFT commented 7 years ago

We were hoping for it to be here to reduce the amount of refactoring necessary. However, I can buy the argument that it would be too confusing or allow someone to easily break their test runs.

AbhitejJohn commented 7 years ago

@JoshCollinsMSFT : I see. Then I think the question would be, "what is the right default?". Is parallelization at the TestClass level where all TestClasses are running in parallel and methods in them are running serially the default for most scenarios? In your case, what would work best?

ChristopherHaws commented 7 years ago

@AbhitejJohn Maybe the default should be assembly level so that there is no change of users? Then users could opt in to parallelization. This would prevent this feature from being a breaking change.

AbhitejJohn commented 7 years ago

@ChristopherHaws : Oh yes, in-assembly parallelization is off by default. so this wouldn't be a breaking change. From the RFC this would primarily be turned on via the TestParallelizationLevel setting in either the runsettings or at an assembly level. When we have this turned on, the question was whether running all TestClasses in parallel was the right default as opposed to (say) running all test methods, irrespective of what TestClass they belong to, in parallel. The former seemed the most convenient pivot to parallelize in terms of minimal inter-dependency of tests and minimal refactoring required to enable this.

ChristopherHaws commented 7 years ago

@AbhitejJohn As a user, I would have assumed the default was the most parallel (TestMethod) if TestParallelization is not explicitly set. I think that doing it any other way would simply be confusing to a new user.

I also think it's good idea to start out with the highest level of parallelism so that new test projects get authored in the most parallel manor (aka discourage the use of statics in test classes). I feel like the TestParallelization setting should be used as a mechanism for decreasing the level of parallelization due to legacy code.

Also, is there talk about having a way to specify that multiple tests must not run in parallel together, but can still run in parallel with other tests? In the SpecRun test adapter you can assign trait to tests and you can configure your test threads to only run certain categories.

In MSTest, it would be nice to be able to specify that certain traits must run sequentially together, but can run in parallel with other tests. I could see it looking something like:

// Run tests in CategoryA sequentially in one thread.
[assembly:TestParallelizationCategoryAffinity("CategoryA");

// Run test in CategoryB sequentially in one thread.
[assembly:TestParallelizationCategoryAffinity("CategoryB");

// Run tests in CategoryC and CategoryD sequentially in one thread.
[assembly:TestParallelizationCategoryAffinity("CategoryC", "CategoryD", ...);

With something like this, you could accomplish test runs like this:

Thread 0: Test1WithNoCategory -> Test2WithNoCategory -> Test3WithNoCategory
Thread 1: Test1WithCategoryA  -> Test2WithCategoryA  -> Test1WithCategoryB  -> Test2WithCategoryB
Thread 2: Test1WithCategoryC  -> Test1WithCategoryD  -> Test2WithCategoryC  -> Test2WithCategoryD
AbhitejJohn commented 7 years ago

@ChristopherHaws : Thanks for that feedback. I see your point on maximum parallelization and I think it does make sense. That would probably be a better default.

On the grouping of parallelizable tests, we did give this some thought and this was falling more into an advanced phase of this support. We plan to wait for more feedback before heading that direction.

/cc @pvlakshm

ivonin commented 7 years ago

Any news on this?

Am I correct in my understanding that there is currently no way to use MSTest v2 and run tests from the single assembly in parallel?

As I see it, MS Test v2 doesn't support .testsettings config which has a parallelTestCount option and .runsettings configuration doesn't allow that.

Should we avoid migrating to MS Test v2 as long as this is the case if parallel test execution is required?

Steinblock commented 7 years ago

@ivonin

 ========== Run test finished: 3 run (0:00:31,2914136) ==========

That's a poor result.

Not what you would expect at first. Why: Because the setting only affects different test projects that are run simultaniously in their own appdomains. To verify that:

Now with MSTestV1 you could create a name.runSettings file with the content

<?xml version="1.0" encoding="utf-8"?>  
<RunSettings>  
  <!-- Configurations that affect the Test Framework -->  
  <RunConfiguration>  
    <MaxCpuCount>0</MaxCpuCount>  
  </RunConfiguration>  
</RunSettings>  

MaxCpuCount: 0 = All cpus or 4 to limit to 4 concurrent threads.

This value is ignored by MSTest V2. You can verify this by choosing the file and running your tests again. Still 30 seconds.

I believe just making mstest v2 running tests in parallel would not be the best choice. Currently I am using mstest v1 with multiple test projects in one solution. A normal test run takes 90 seconds. With "enable parallel test execution" I could reduce the time to 30 seconds (the biggest test project takes 30 seconds) which is great.

But if I enable mstest parallel execution with a runSettings file improves execution even more. But the main difference is that this does not isolate the tests. So my test projects use another class library which is not thread save and half my tests fail.

The best solution would be that vstest engine would run every test in it's own isolated test domain if I choose "Run tests in parallel"

And another thing: If I write

        [TestMethod]
        public async Task TestMethod1()
        {
            await Task.Delay(10000);
        }
        [TestMethod]
        public async Task TestMethod2()
        {
            await Task.Delay(10000);
        }
        [TestMethod]
        public async Task TestMethod3()
        {
            await Task.Delay(10000);
        }

I would expect to get a better performance since the scheduler could switch to the next test, but that's not the case. This could be something that the dev team could implement so everybody could write async tests.

So what are your choices?

ivonin commented 7 years ago

Thanks for your analysis. Yes, that's what I see as my options at the moment.

MaxCpuCount in .runsettings is not the same as parallelTestCount in .testsettings. There is no alternative to the latter in MS Test v2 as far as I can see.

What I want is to hear from the project maintainers whether it's a final design decision or if it's going to change in the future.

AbhitejJohn commented 7 years ago

@ivonin: would the workaround from this comment work while we get in-assembly parallelization up?

ivonin commented 7 years ago

@AbhitejJohn: it feels very odd to include somethig like this in our testing process, both on developers machines and on a build server. I'll try it in my spare time but it feels like we'll have to stick with MSTest v1 for the time being (which is unfortunate, by the way, because it's one of the things that keeps us from migrating to .NET Core).

KthProg commented 6 years ago

For the love of Christ please implement this.

AbhitejJohn commented 6 years ago

@KthProg : Thanks for that extra push! We started off on this and hope to have something out soon.

vincez commented 6 years ago

Nudge.

Thwaitesy commented 6 years ago

@ivadim @Steinblock - If you wanted to use the new vstest .runsettings format but still keep the parallelTestCount nature then you can achieve it like this:

  1. Create a file called "parallel.runsettings" with the contents being:
<?xml version="1.0" encoding="utf-8"?>
<RunSettings>
  <RunConfiguration>
    <MaxCpuCount>0</MaxCpuCount>
  </RunConfiguration>
  <MSTest>
    <SettingsFile>parallel.testsettings</SettingsFile>
    <ForcedLegacyMode>true</ForcedLegacyMode>
  </MSTest>
</RunSettings>
  1. Then create another file called "parallel.testsettings" with the contents being:
<?xml version="1.0" encoding="UTF-8"?>
<TestSettings name="Parallel" id="5050e51f-1e27-4d16-b024-0b072e54c2b5" xmlns="http://microsoft.com/schemas/VisualStudio/TeamTest/2010">
  <Deployment enabled="false" />
  <Execution parallelTestCount="50" />
</TestSettings>
  1. How to use the settings file:

    Visual Studio: Select the test settings file (parallel.runsettings) - Test > Test Settings > Select Test Settings File

    VSTS: Use the VSTest Task and point the test settings file to the "parallel.runsettings" file you created earlier

This achieves:

Issues: If your tests are not thread safe, this won't work 😢

Hopefully this helps someone 👍

PureKrome commented 6 years ago

@AbhitejJohn any update on this issue?

JoshCollinsMSFT commented 6 years ago

Looks like the work item was committed. Should be published in the testfx myget feed.

PureKrome commented 6 years ago

Also @AbhitejJohn / People working on this item, we (the developers) need some blog post/announcement please about all of this, once this stuff goes live to the public.

So we know how to do things properly.

Some people don't want parallelization while others (me!) love to parallel-all-the-things :)

KthProg commented 6 years ago

Well it's certainly hard to use this as the backing for CI UI tests without it. Without parallel execution there's not much C in CI. Our very simple tests right now run over 8 minutes. With parallelization it would take about 3.

pvlakshm commented 6 years ago

It is published on the myget feed now: https://dotnet.myget.org/feed/mstestv2/package/nuget/MSTest.TestFramework/1.2.0-build-20171130-03 https://dotnet.myget.org/feed/mstestv2/package/nuget/MSTest.TestAdapter/1.2.0-build-20171130-03 These are early bits. Try it out, and let us know.

kabilanvk commented 6 years ago

I tried the solution but still the tests are running sequentially. My .runsettings file is like this, `<?xml version="1.0" encoding="utf-8"?>

4 MethodLevel `
jayaranigarg commented 6 years ago

@kabilanvk : Are you running your tests in some CI/CD pipeline or just running them locally using command-line(vstest.console) or IDE? If your tests are running in parallel, you should see an information message like this in your console(command-line) or output window(IDE): MSTest Executor: Test Parallelization enabled. Workers: 4, Scope: MethodLevel.

If not, can you make sure you are using adapter and framework bits with version 1.2.0-build-20171130-03 ?

kabilanvk commented 6 years ago

I don't see the log as you mentioned. I'm using VS 2017 and my package config is, <package id="MSTest.TestAdapter" version="1.2.0" targetFramework="net45" /> <package id="MSTest.TestFramework" version="1.2.0-build-20171130-03" targetFramework="net45" />

I enabled diagnostic logs for test in VS 2017 options. Here is the runsettings log before execution starts, `[12/12/2017 5:30:39 PM Diagnostic] RunSettings Content:

4 MethodLevel C:\Working\P4_Automation\src\TestResults C:\Working\P4_Automation\src\ X86 Framework45 `
jayaranigarg commented 6 years ago

@kabilanvk : Apologies for the confusion. You will have to update the adapter as well: https://dotnet.myget.org/feed/mstestv2/package/nuget/MSTest.TestAdapter/1.2.0-build-20171130-03

Can you please try updating it and see if it works for you?

kabilanvk commented 6 years ago

Awesome Thanks, I'm now able to run my basic tests parallel. Will try to run more detailed tests and come back if there is any issues.

pvlakshm commented 6 years ago

We have posted a sample. Try it out and let us know your feedback.

andersforsgren commented 6 years ago

I'll try to ask again because my question seem to have slipped under the radar before: is safe parallel execution of tests without modification within scope for this issue? I.e. will it pass this test suite successfully in 10 seconds? If not - is there another issue for this?

static class StaticThing
{
    private static int count;
    public static int Count() => ++count;
}  
[TestClass]
public class TestClass1
{
    [TestMethod]
    public void TestMethod1()
    {
        Thread.Sleep(10000);
        Assert.AreEqual(1, StaticThing.Count());
    }
}
[TestClass]
public class TestClass2
{
     [TestMethod]
     public void TestMethod2()
     {
         Thread.Sleep(10000);      
         Assert.AreEqual(1, StaticThing.Count());
     }
}
jayaranigarg commented 6 years ago

@andersforsgren : Apologies for missing this.

I am afraid the above suite is not parallel ready. You have to make sure that your code is thread-safe. Parallel execution for non-parallel code is beyond the scope of this issue. You can file a new issue for this feature request and we will try to address it depending upon its feasibility and number of people interested in that feature.

andersforsgren commented 6 years ago

@jayaranigarg OK. Will do that. I'm surprised this isn't one of the proposed "modes" - as it's a much simpler and smaller scope task than actually running things concurrently within the appdomain (Tests are just batched to N workers in their own appdomains, instead of N threads).

jayaranigarg commented 6 years ago

@andersforsgren : Here is the complete RFC. Please feel free to read and provide feedback.

andersforsgren commented 6 years ago

Should I provide the feedback somewhere else, e.g. to the RFC rather than here? I commented above (first comment after the "please provide feedback" comment)

My feedback is, again:

pvlakshm commented 6 years ago

The vstest runner already supports the level of parallelism you mention. Please see this blogpost, and the documentation on msdn. Let me know if I understood your feedback correctly.

andersforsgren commented 6 years ago

Thanks, no not quite:

Process parallel execution is not very useful if it requires the author to arbitrarily balance the size and number of test assemblies. For the process parallel execcution to be useful it needs to be able to take M test containers and run them on N cores, and keep the cores busy regardless of the size and number of containers. E.g. M can be just 1 container, or M can be 2 where the first assembly has 10000 tests and the second has just 10.

So I'll clarify this feedback further: as a test author I don't want to modify my production code nor my test code to satisfy the test executor:

Executors that do this properly simply read all test methods from all assemblies, and then instruct the workers to take chunks from the full set of tests. Afterwards the orchestrating process produces a complete test result. So while we have several ways of achieving parallelism (fine grained threading parallelism per class or method, or process parallelism per container) - the one feature that would make it 10x more useful would be the combination of both: per-testmethod parallelism with separate processes.

I might have misunderstood the functionality that's already there, but I haven't been able to achieve this in any of the existing test runners at least.

KthProg commented 6 years ago

My reply to this is: too bad? That functionality already existed. Parallelism between multiple cores is obviously much different than between multiple processes and in terms of speedup only the first provides a real genuine speedup (in fact splitting between processes is much slower, because of the time to switch between processes on the same core). You can easily call the test runner with specific groups of tests from the CLI to run them under multiple processes.

It only took me about 1 day to update my code to be parallelizable and I think it's an extremely useful feature and changing code in that way is not that challenging.

If what you want is to be able to run non-thread safe code in parallel then I think you'd find there is no way for them to do that in a way that handles all cases. They can't just spin up separate app domains to handle my selenium tests. They all work with the same web application and will interfere with each other, so making the code thread-safe is necessary anyways. TLDR; there's no generic way for them to do that that works for everyone's code, it's much better to put the onus on the dev to make it work with parallel execution.

andersforsgren commented 6 years ago

splitting between processes is much slower, because of the time to switch between processes on the same core

The idea is running N processes when you have N cores. Obviously the OS does other things at the same time, but the idea is that 1 process runs on each core, and the core stays as busy as possible.

It only took me about 1 day to update my code to be parallelizable and I think it's an extremely useful feature and changing code in that way is not that challenging.

It would take tens of years for me to do across our code base (which is over 200 man years). The test suite of 25000 tests can take 20 minutes to run using a custom test runner that saturates all cores, and 1.5 hours to run in vstest. We are in different situations there, obviously.

it's an extremely useful feature and changing code in that way is not that challenging.

In general it's neither desireable nor possible to make code that won't be executed in parallel (in production) thread safe. For example, changing a Dictionary<K, V> to a ConcurrentDictionary<K,V> (or adding the equivalent locks) introduces a nontrivial performance overhead. And even if you did this, you probably failed in a few places (because this is hard). So you end up having test failures which show up as flaky flip-flopping tests due to race conditions, and that are also false positives because in production you run on a single thread. Not a good situation to be in.

I think you'd find there is no way for them to do that in a way that handles all cases.

I completely understand that. My point is merely that "1 process per core" is conceptually very simple - existing test runners that do this are very little code, and it offers a benefit over all the other threading modes, namely that it runs the production code on multiple without modification - so long as it's independent of external factors (such as web browsers, databases). A typical suite of unit tests will fit this description.

The mode I'm describing is almost exactly the "process parallel" mode that is already described (and has existed for a long time). The suggestion is merely a small and easily implemented improvement - that the unit of parallelism isn't one assembly. All the test runner has to do is split the work in smaller chunks ("virtual test assemblies" if you want) - in order to keep the CPU cores busy.

KthProg commented 6 years ago

Honestly I already disagree with my own post lol. Our test suite is not that large and fairly simple. So yes in those cases I could see it being a huge issue. And splitting each process onto a separate thread is certainly possible. I didn't think about things like collections. I'd think that both modes should be offered, and I take your point that conceptually it's not hard to implement for the benefit.

PureKrome commented 6 years ago

@aanurca - this might be starting to get a bit offtopic, but .. can you switch from Log4Net to something more modern like Serilog or the built in Logger in .NET Core? Might find things working better, now...