Fix problem with parallel loading and limitations on number of agents #141
Conversation
|
This (re) fixes #36 |
| @@ -54,7 +54,7 @@ public MasterTestRunner(IServiceLocator services, TestPackage package) | |||
| _runtimeService = _services.GetService<IRuntimeFrameworkService>(); | |||
| _extensionService = _services.GetService<ExtensionService>(); | |||
| _engineRunner = _services.GetService<ITestRunnerFactory>().MakeTestRunner(package); | |||
| LoadPackage(); | |||
| //LoadPackage(); | |||
There was a problem hiding this comment.
I'm concerned that without this line we may actually cause a regression of #52 since LoadPackage is never called now
There was a problem hiding this comment.
We need to get @NikolayPianikov to comment on this as well, but it would still be useful if you could check out how it is working in the case of loading multiple assemblies, without necessarily having a project for the.
According to #52, the problem was not that the TestPackage needed to be loaded but that it needed to be expanded. MasterTestRunner uses it's method EnsurePackagesAreExpanded to do that. It might be that we have to call it separately from LoadPackage - possibly in the constructor.
We don't want the package loaded for sure. Loading means loading the assembly and finding all the tests. In the case of multiple processes, it implies creating all the processes, which might amount to 100s of processes for some projects.
While waiting to hear from @NikolayPianikov, let's each test as we can. I'll try running the two assemblies through an NUnit project to see what happens.
|
I see why @NikolayPianikov added the call to LoadPackage now. It does a bunch of stuff in addition to loading the package, including expanding any projects. So I renamed the method to InitializePackage, removed the loading itself and am calling it. This seems to work and gets us back to the desired behavior where all loading is done by the TestEngineRunners that MasterTestRunner uses. I'm not completely happy with having this call in the constructor, but I can't see any better place to do it. |
|
I've tested this in our environment and can confirm it is working as expected @CharliePoole I'm going to continue letting it run and will let you know if I see any other race conditions cause issues. |
|
Thanks Chris... I'll wait a bit before merging. @NikolayPianikov do you have any comments? |
|
Ran it constantly for the last couple hours, no issues |
|
I'll merge this then. When you have rebased, I'll take another look at your PR. |
|
With this change my other PR isn't necessary, I'm going to close it |
|
@chris-smith-zocdoc I'm not sure that's right. See my comment on PR #140 |
A problem crept into the engine causing all assemblies to be loaded as soon as the MasterTestRunner was created. Of course, this causes all processes to be created at once, in spite of any limitation the user may have set with the
--agentsoption.After removing the offending line of code, I discovered that there were random failures occuring because of threading issues in RuntimeFramework. This will need a lot of work at some time, so I added an explanatory comment to RuntimeFramework. For the moment, the hack I added to RuntimeFrameworkService prevents the problem from occuring by forcing RuntimeFramework to initialize the list of available frameworks early enough that it's ready for use when multiple threads start trying to access it.
@chris-smith-zocdoc This is the fix you will want to have in your own PR. Since you are set up to test the situation of loading multiple assemblies in parallel, could you pull this and try it out? I have been running with only two processes and various command-line options. I haven't seen the exception thrown for the last dozen or so runs, so I'm fairly confident, but it's hard to test a negative!