Load packages in parallel

[chris-smith-zocdoc]

Allow the AggregatingTestRunner to perform LoadPackage in parallel for each runner

Opening this up for discussion of #132

I'm open to alternate approaches if we think I should make these changes somewhere else.

[CharliePoole]

I'm not sure this is the way we want to go for #132 although it is something we may need in the future.

Loading all the test assemblies in parallel would definitely speed up a load or explore operation. That's something we do in the GUI because we need to display all the tests. It would also help in the console runner when the --explore option is used across some large number of assemblies, although that's probably not such a common use case.

But in the normal use of the console runner - running tests - we do not want to load all the tests before we run any tests. Instead, we want each assembly to be loaded just in time - right before the tests are executed. That way each process is created, its tests are loaded and then run and the process terminates after returning the result. We had this working at a point - a few months back, I believe - but some changes came in and broke it. We should get this working again.

The change in the current PR may appear to help right now because the execution of tests is broken in such a way that LoadPackage is called on the AggregatingTestRunner. The problem is that it is not supposed to be called! Instead, each individual LoadPackage should be getting called when we try to run the tests.

My preference would be to hold this fix until we fix the original problem that is caused by use of AggregatingTestRunner.LoadPackage and then retest this one.

[chris-smith-zocdoc]

Thanks for the clarification @CharliePoole

We had this working at a point - a few months back, I believe - but some changes came in and broke it. We should get this working again.

Do you have a link to those changes so I could pick up that work? I like that approach better also

[CharliePoole]

@chris-smith-zocdoc I don't have a link. I've been meaning to do a bisect in order to find the changes that broke it. I'll take a look tomorrow and see what I come up with.

[chris-smith-zocdoc]

@CharliePoole The breaking change appears to be introduced here #52

Specifically the call to LoadPackage(); was introduced into the constructor of MasterTestRunner

I tested a commit before that change 48bee7a and verified the correct behavior was present

[CharliePoole]

@chris-smith-zocdoc Thanks for that! My day is just starting, so I had not gotten to it yet. I'll take a closer look and we can figure out how to get a fix in as well as how to handle your PR.

The general idea is that we don't want to load all the assemblies at once, unless the runner actually asks the engine to do that. This would probably be by calling Explore or possibly Load. The console runner no longer does that, relying on the Run command to load tests as a side effect.

[CharliePoole]

OK, I took a look. That's quite wrong: merely constructing a runner shouldn't cause so much to happen.

As far as I can tell, the other calls to load package were not removed so I think that line can be removed. Either you or I can do that small change and test it.

[chris-smith-zocdoc]

I can make that change and rebase this branch

[CharliePoole]

Do it as a separate PR so we can merge separately.

[chris-smith-zocdoc]

Actually, on second though, it would be helpful if you could test this out as well. Just removing the LoadPackage() call from the constructor doesn't appear to be enough

[CharliePoole]

OK, I'll do that

[CharliePoole]

Here is an old comment by Rob that suggests the issue was introduced at a different point: #36 (comment)

It's entirely possible that we keep breaking and re-fixing the max agents functionality because we don't have an adequate test for it! In the past, I have verified the sequence by examining the internal trace log.

[CharliePoole]

Here is an old comment by Rob that suggests the issue was introduced at a different point: #36 (comment)

It's entirely possible that we keep breaking and re-fixing the max agents functionality because we don't have an adequate test for it! In the past, I have verified the sequence by examining the internal trace log.

[CharliePoole]

@chris-smith-zocdoc

So, I started from the latest master and commented out that single line in the MasterTestRunner constructor that calls LoadPackage(). I ran the command...

nunit3-console nunit.engine.tests.dll nunit3-console.tests.dll --agents=1 --trace=Info

The log nunit-agent_10876.log contains these lines...

20:19:10.498 Info  [ 5] NUnitFrameworkDriver: Loading nunit.engine.tests.dll - see separate log file
20:19:11.051 Info  [ 5] NUnitFrameworkDriver: Loaded nunit.engine.tests.dll
20:19:11.052 Info  [ 5] NUnitFrameworkDriver: Running nunit.engine.tests.dll - see separate log file

The log nunit-agent_14192.log contains this...

20:20:03.358 Info  [ 5] NUnitFrameworkDriver: Loading nunit3-console.tests.dll - see separate log file
20:20:03.793 Info  [ 5] NUnitFrameworkDriver: Loaded nunit3-console.tests.dll
20:20:03.794 Info  [ 5] NUnitFrameworkDriver: Running nunit3-console.tests.dll - see separate log file

That indicates to me that the tests are running properly. The loading is just in time and only one process is used at a time.

However, if I remove the --agents argument, I get an error saying the 4.5 framework is not available and only the first assembly runs. I'm debugging.

[CharliePoole]

Hmmm... problem seems to be intermittent - I just reran successfully.

[CharliePoole]

When the crash does not occur, the loading and execution run in parallel:

First assembly:

20:56:06.533 Info  [ 5] NUnitFrameworkDriver: Loading nunit3-console.tests.dll - see separate log file
20:56:07.028 Info  [ 5] NUnitFrameworkDriver: Loaded nunit3-console.tests.dll
20:56:07.030 Info  [ 5] NUnitFrameworkDriver: Running nunit3-console.tests.dll - see separate log file

Second assembly:

20:56:06.533 Info  [ 5] NUnitFrameworkDriver: Loading nunit.engine.tests.dll - see separate log file
20:56:07.100 Info  [ 5] NUnitFrameworkDriver: Loaded nunit.engine.tests.dll
20:56:07.102 Info  [ 5] NUnitFrameworkDriver: Running nunit.engine.tests.dll - see separate log file

This looks OK to me - except for the crash, of course.

[CharliePoole]

RuntimeFramework has a couple of static fields that use non-threadsafe lazy initialization. I think that's what is causing me to lose some available frameworks, leading to the error.

[CharliePoole]

@chris-smith-zocdoc You commented elsewhere that this PR may no longer be necessary if we use JIT loading for execution. However, I'm not sure that's right. What's happening for Explore() with the current master?

[chris-smith-zocdoc]

Explore still runs serially. Parallel execution is only implemented for the Run method on ITestRunner on AggregatingTestRunner

Link to Explore

[ChrisMaddock]

Had a little dig into this area recently, which reminded me of this. Sounds like the just-in-time assembly loading solves the particular case you were looking at previously @chris-smith-zocdoc - but I still think this would be a good efficiency to get in for load, and for explore and count etc.

IMO, this should be blocked by #168 - the underlying functionality of Load isn't threadsafe, and pulling this in may affect runners which actively call the Load method. (I'm not sure what the GUI and adapter do here.)

Ideally #173 would also be resolved before much further work, the current implementation is a little hacky, and a potential source of flaky failures if the wrong changes are made.

[CharliePoole]

@ChrisMaddock I agree with the necessary order of resolving issues that you state: #168, #173 and then this one.

@chris-smith-zocdoc What you'll need to do is bring this up to date with other changes that have been made. I'd try rebasing first. Of course, you may need to do it again after #168 is done.