Repository navigation
Where should TestPackage be defined? #919
Description
Question for the team for V4...
Currently, TestPackage is defined in the nunit.engine.api assembly. I'm working on issue #889 to remove some confusion about the constructor for TestPackage and simplify some other things I'm working on.
But it seems to me that there's a bigger question... Should TestPackage really be defined as a class in this assembly, which is made up almost entirely of interfaces?
So what are the alternatives? I can think of two:
- Define an
ITestPackageinterface in the api assembly and defineTestPackageitself in the engine, probably inengine.core. That would mean giving the user some new interface (maybe a service) to create test packages. - Define an
ITestPackageinterface that exposes all the functionality we need and have the user define it in their application.
Or we could keep doing it as we now are.
Throwing this out there for consideration, since we are planning on changing the engine interface anyway.
Activity
ChrisMaddock commentedon Mar 27, 2021
Interesting point. I'm not sure I can come up with a reason to feel particularly strongly either way.
I agree its inconsistent, however TestPackage is such a fundamental class I almost feel like it's allowed to be inconsistent? From my memory there's only two classes a runner should instantiate - TestPackage and TestEngine (Indirectly through TestEngineActivator).
Other than consistency, I'm not sure I can think of a significant advantage to putting TestPackage behind an Interface - I believe the exposed methods are quite limited, and I can't think of a particular need to mock it in any way.
My only thinking is that changing this would be a breaking change for even the simplest of runners. No problem at all with making breaking changes in 4.0, but I'm less sure on making such a fundamental one unless we have a significant need to do so.
ChrisMaddock commentedon Mar 27, 2021
Here's a thought - will this be affected at all by our plans to split up the API assembly for extensions to help with the versioning?
We talked about whether we'd need a nunit.engine.api.code.dll assembly. If we have a concrete TestPackage class, that would be big enough and change enough regularly that we definitely would, and also require all the associated extension.api.dll assemblies to be rereleased to reflect those changes. If we had a super simple ITestPackage interface, it wouldn't.
You might be onto something here, Charlie! 😄
CharliePoole commentedon Jun 28, 2025
@nunit/engine-team This is four years old and I'd like to get something done in the next beta - either close it without action or make a choice and implement it.
I prefer option 1 over 2. Requiring the user to define their own class seems a bit unfriendly. But I'm not adverse to leaving it alone either.
manfred-brands commentedon Jun 28, 2025
@CharliePoole Option 2 doesn't seem logical.
An interface allows multiple implementation, which I don't think we need.
It also allows hiding parts of the implementation.
At the moment
SubPackagesis exposed as anIListallowing users to add packages after creation, is this the intention?The main usage in this repo is the recursive ReadXml method.
We talked about splitting
TestPackageinto a plural and a singular. So we don' t need guards on!HasSubPackages()or have code like:TestPackage package = new TestPackage(testFile).SubPackages[0];I don't see any real usage of the nested packages, they are flattened with
Select.In what cases would we have nested package and is this preserved in any output?
If we only need one level we can simplify the code in a TestPackages (group) and TestAssembly (or other name)
We could have a read-only interface/class used by the runners/engine to clearly indicate that a package is input and not modified.
We then can have a factory (method) to create a package from either XML or a set of files/settings.
CharliePoole commentedon Jun 28, 2025
@manfred-brands
I think we can forget about option 2.
Historically we had two constructors with a subtle difference that constructing with one file gave you a named package, while a list of files gave you an anonymous package containing a named subpackages. Every runner method required testing against both types of packages. We wanted to get rid of that for V4 and decided that the users should only be able to create a top-level anonymous package with one or more named subpackages. We obviously could have picked a different approach, but that's the one past efforts were driving toward. We could still change to a two-class approach.
No, I don't think it was. And if users can only create top-level, unnamed packages, adding one of those could be an error. Probably this should be readonly or just an IEnumerable. We used a List because of the need to operate on runtimes without Linq available.
I think the case relates to projects. If the command-line includes multiple files and some of them are project (nunit, or VS) then we want to be able to give a result that summarizes nested projects. In 2019 I added code so that the lower-level runners called by MasterTestRunner didn't need to know about projects while the output still gave aggregate results for each test project. This is unneeded from the perspective of the console runner but useful in a runner that displays the hierarchy. I haven't looked at the code for a while but IIRC, MasterTestRunner uses that nested hierarchy of packages to reassemble the flat list of assembly results into a corresponding hierarchical result.
Except it is modified now, internally by the engine, and those modifications are used by the console runner for displaying information placed in the Settings by the engine.
Bottom line, I think we want the TestPackage to be easy to create by the caller and modifiable only in certain ways. It also needs to be modifiable by the engine to carry information along with it about things like the targeted runtime. Finally, some of that information needs to be accessible by the agents, so the implementation code has to be either in the API assembly or the common assembly. At one point, I had the seralization code in the API assembly, which seemed too big. Right now, the code for TestPackage itself is fairly minimal but it also pulls in classes PackageSetting, PackageSettings, Setting Definition and SettingDefinitions. Maybe we need to look at some of those classes as well.
CharliePoole commentedon Jul 3, 2025
I'll focus initially on those additional classes, to see if they can be easily moved:
SettingDefinitionsis not used by the api at all. It's a static class listing all the standard setting definitions we support. Keeping it in the api assembly means introducing a change to the api every time we define some new setting name. I'll move it to thenunit.commonassembly.UPDATE: This isn't as simple as I may have thought. See issue #1727
CharliePoole commentedon Jul 3, 2025
Not seeing any further advantage to a change right now and having created issues for some related changes, I'm closing this as resolved. We'll keep it where it is.