v4: Create single constructor method/structure for a TestPackage

[ChrisMaddock]

From memory, there are currently multiple constructors for a test package, which create different package sturctures which the agent must handle. We should reduce this to a single method.

[CharliePoole]

Decided I should do this before starting issue #909 spike!

[ChrisMaddock]

Great!

This issue is the epitome of technical debt in my opinion. The number of times it's made something I've been trying to do harder!

[CharliePoole]

Thoughts on how to do this. I'd appreciate comments from @nunit/engine-team and others.

We have two constructors because

• The top level package representing the command-line is anonymous (FullName = null) and has a non-empty list of subpackages. The array constructor handles this.
• Subpackages themselves are created using a string argument, which is the name of the subpackage. Note that the individual items on the command-line are subpackages of the anonymous top-level package.
• If any of those individual items are supported project types, they will be expanded to include a list of subpackages. This means that the tree of packages will is always either two- or three- deep. (Of course, someone could invent a package at some point, which is able to contain other packages.)

Options considered...

• Eliminate the array constructor. By itself, this gives us no way to represent the command-line.
• Eliminate the string constructor. By itself, this gives us no way to create sub-packages.
• Use separate classes for each of the two uses. This could be done in several ways, but I lean toward removing the FullName property from TestPackage and deriving NamedTestPackage from it.

Personally, I like the third option.

Charlie

[ChrisMaddock]

I agree with the third option. Some other thoughts:

• Is it correct that only one of these methods should be publicly exposed, meaning we can prevent runner-maintainers from "using this wrong"? I'm not sure how ProjectLoader's work off the top of my head, can they return a normal package that is then attached as a subpackage?
• Separate classes sounds ideal to enforce type safety. Would it be more explicit to name the second class something like SubPackage - which removes the ambiguity (to me, at least!) about when packages should and shouldn't have a name?
• With this, we're defining that a test package always has at least one subpackage. That feels a little odd, that a simple one assembly test requires a package and a subpackage - but I like that it would make things consistent. It almost feels like "TestPackage" is now a very differnet thing from a SubPackage - in that the top-level package will never have it's own assembly. Kinda feels like two totally separate classes now.

Whatever we settle on, definitely think we should get this documented! Something I'm still not sure about in the current codebase is what the structure of a TestPackage should or could be - which makes it very difficult to reason about!

[CharliePoole]

@ChrisMaddock

If we forget about fields and properties and just keep it to the conceptual level, a package is a collection of assemblies OR other packages, together with settings, which apply to contained packages unless overridden. If you consider it that way, then a package doesn't need a name unless we need to refer to it.

I thought of making the subpackage class internal. But for test purposes, we often construct subpackages and then add them to a package. This might be something a calling program would want to do as well. It seems cleaner if everything is a TestPackage and any name just gets ignored when the top-level package is passed to the runner.

I lean toward NamedPackage because it says what the thing is rather than how we use it. If someone wants to construct a tree entirely out of unnamed packages, we might not care. IMO the important thing is that they are constructed consistently.

That said, maybe the naming should be based on what the package contains like AssemblyPackage, ProjectPackage. I can play with it a bit when I work on it (which will be after #913 merges).

There's a reason why a package always has one subpackage. Conceptually, it's inherited from NUnit V2, where the command-line was always an NUnit project. That is, even if you entered a single assembly, what got was an unnamed project containing a single assembly. Same goes for NUnit3. If you enter an assembly you get a package with a single assembly subpackage in it plus settings derived from your command-line options. It's actually pretty powerful.

I think the most important thing we have to do is to stop treating the two constructors differently.

[mikkelbu]

Should this be closed as #920 was merged?

[CharliePoole]

Yes, we don't get automatic closing on the 4.0 branch.

[CharliePoole]

This was merged to dev-4.0 and closed. I'm reopening it so as to replicate it in main.