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

Load/UnLoad packages in parallel #132

Open
chris-smith-zocdoc opened this issue Nov 16, 2016 · 8 comments
Open

Load/UnLoad packages in parallel #132

chris-smith-zocdoc opened this issue Nov 16, 2016 · 8 comments

Comments

@chris-smith-zocdoc
Copy link
Contributor

When using multiple process / domains it would be nice to have them load/unload in parallel instead of serially. For large test suites with many dlls this can be a large performance gain.

See AggregatingTestRunner

We could take this a step further and make all operations run in parallel, the additional missing ones would be

  • CountTestCases
  • Explore
  • Dispose

I'd be happy to take this work on after some discussion here

@CharliePoole
Copy link
Member

Let's figure out first exactly what is happening now. There are two primary use cases: with and without setting the max number of agents. After the recent changes to handling the --agents option, I'd expect loading to be done in parallel. It would be good to set up a test that shows whether that's true.

@chris-smith-zocdoc
Copy link
Contributor Author

Interesting, I'll look into that now

@chris-smith-zocdoc
Copy link
Contributor Author

@CharliePoole Using the latest master 021cdac I still observe the serial loading behavior

@CharliePoole
Copy link
Member

Can you get a log that shows where it's happening?

@chris-smith-zocdoc
Copy link
Contributor Author

I'm seeing this call stack when I run nunit3-console under dotTrace

image

Link to the code

protected override TestEngineResult LoadPackage()
{
var results = new List<TestEngineResult>();
var packages = new List<TestPackage>(TestPackage.SubPackages);
if (packages.Count == 0)
packages.Add(TestPackage);
foreach (var runner in Runners)
results.Add(runner.Load());
return ResultHelper.Merge(results);
}
/// <summary>
/// Unload any loaded TestPackages.
/// </summary>
public override void UnloadPackage()
{
foreach (ITestEngineRunner runner in Runners)
runner.Unload();
}

The command I'm using

nunit3-console <list of dlls>

After the dlls are loaded, they do get run in parallel.

@chris-smith-zocdoc
Copy link
Contributor Author

@CharliePoole Any more thoughts on this issue? I have a proof of concept change on this branch https://github.com/nunit/nunit-console/compare/issue-132

@CharliePoole
Copy link
Member

Sorry, I was out of town all last week and haven't looked at this yet. Will try to do it today. From a quick glance at your changes, they seem to mix the solution to the current problem with some changes that we may need in the future - when we have a runner that uses the count and load facilities independently of actually running the tests. I'm always hesitant to build in functionality that we think will be used in the future, rather than waiting to see which future arrives. :-)

@chris-smith-zocdoc
Copy link
Contributor Author

From a quick glance at your changes, they seem to mix the solution to the current problem with some changes that we may need in the future - when we have a runner that uses the count and load facilities independently of actually running the tests.

Certainly not my intention, I was trying to balance duplicating the parallelism code with making it a little more abstract.

Do you think AggregatingTestRunner is where I should be making my changes? If so I can open a PR and we can discuss the details there, otherwise I can explore alternate approaches.

@CharliePoole CharliePoole added this to the Future milestone Jan 13, 2022
@CharliePoole CharliePoole modified the milestones: Future, Future Ideas Oct 3, 2024
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