Remove CreateManyConcurrent test

[ianhays]

Adds a conditionalFact to the CreateManyConcurrentInstances test so we don't run the test when max_user_instances is greater than 200. That's just too many instances to create and has the potential to cause OOM exceptions. I also cleaned up the project and solution files to play nicely with VS.

The test is causing intermittent failures in other tests and is also intermittently failing itself. Without modifying system resources we can't make this test trustworthy, so it's better to remove it for the sake of stability.

resolves https://github.com/dotnet/corefx/issues/11622
resolves https://github.com/dotnet/corefx/issues/5660

[stephentoub]

we don't run the test when max_user_instances is greater than 200

Do the CI machines have a value smaller than that? If yes, then will we get coverage in CI? If no, then is this fixing the problem?

[ianhays]

Do the CI machines have a value smaller than that?

Last I checked the limit was 1024 so this test won't ever run, which means we'd lose coverage for this case on the CI machines.

I'm not sure how else to solve this other than modifying the max_user_instances in the test which is awfully invasive. Got any ideas?

[stephentoub]

Last I checked the limit was 1024 so this test won't ever run, which means we'd lose coverage for this case on the CI machines.

And it's outer loop... if this change were to be made, I'd be surprised if this test ever ran.

I'm not sure how else to solve this other than modifying the max_user_instances in the test which is awfully invasive.

Can you help me to understand what's triggering OOM conditions? How much memory is an instance taking up that 1024 is causing OOMs?

[stephentoub]

@ianhays, further thoughts on this PR?

[ianhays]

@ianhays, further thoughts on this PR?

You're right about it being seldom run. We might as well just remove the test in that case.

I'm not fond of tests that verify behavior around system-wide limits like this when we're running tests in parallel. As long as this test is run alongside other tests, we've got potential for intermittent failures. If that were the trade-off, I'd prefer to not have this test than to have this test interfering with others and producing false failures.

We also shouldn't rely on running the test based on what inotify is set to since it will incorrectly fail when inotify is set arbitrarily large from OOM.

That said, we need to do something with this test. I'd be fine with removing it only because it's a test for the system behavior instead of the behavior of the .NET code, but an argument could be made that the exception type we throw merits a test. I only think it's important that we don't compromise stability for the sake of this one test.

In a perfect world we could mark this test with an attribute that disabled concurrent execution across all projects so we could set inotify to 10 and hit the error right away. In our world I'm leaning towards removal, but I'm on the fence.

Edit: Disabling parallel execution for just this project would also be an option, but that's a project-wide change that will slow down all of our tests when running innerloop or outerloop.

[ianhays]

As long as this test is run alongside other tests, we've got potential for intermittent failures

case in point: https://github.com/dotnet/corefx/issues/5660

[stephentoub]

I'd be fine with removing it only because it's a test for the system behavior instead of the behavior of the .NET code, but an argument could be made that the exception type we throw merits a test. I only think it's important that we don't compromise stability for the sake of this one test.

There were two goals for this test: verifying that we do throw an exception when we hit the limit, and verifying that we respect the limit such that if someone raises it, they get the intended benefit of being able to create more.

But as this has system-wide impact, you're right that the test is a bad citizen. Let's just delete it.

[ianhays]

I rebased to remove the test and updated the OP.

[stephentoub]

Thanks, Ian.