Eliminate string constructor for TestPackage #920
Conversation
|
Although this is only a refactoring toward 4.0, I've asked for review because of substantive changes to the tests. I'll be happy if someone just looks those over and agrees that what I've done is the way to go. In fact, the tests of what runner is used in various circumstances represent what is currently done. If that's not what we want done in some case, we should flag that as a separate issue. |
There was a problem hiding this comment.
Hi Charlie,
The changes look good - but I'm wondering if we can come up with a more explicit API than the Named() method - I feel like whether a package is named or not is a consequence of it's place in the structure, rather than key characteristic.
I think I might have stumbled across an alternative while reviewing this - let me know what you think.
| Inconclusive = 2 * 1, | ||
| Skipped = 2 * 7 | ||
| })); | ||
| // TODO: Get extensions working - need new releases due to interface change |
There was a problem hiding this comment.
Is it worth filing an issue for this? I wouldn't want it to be forgotten. 🙂
There was a problem hiding this comment.
I'll be doing at least the NUnit Project as a new release while we work on this. Don't want to do too much until the API is established.
There was a problem hiding this comment.
Actually, it may only be the NUnit Project loader that has a problem... it uses the old string constructor, which now looks like a params string[] constructor!
| Settings = new Dictionary<string,object>(); | ||
| SubPackages = new List<TestPackage>(); | ||
| } | ||
| InitializeSubPackages(testFiles); | ||
| } | ||
|
|
There was a problem hiding this comment.
I thought we already had a params ctor for this! Do we need both constructors, if we're changing the API for v4 anyway? I guess there's no major drawback, other than being a less clean and simple API.
There was a problem hiding this comment.
No, we didn't because it would never have been used for a single string argument - the named constructor would have been used. I added it primarily for ease of use in tests, but then I thought users like ease of use as well. We could have it as the only constructor, but would have to change two or three places where we use the IList constructor. I can go any way you like on this.
There was a problem hiding this comment.
Personally, I prefer the clean API with a single constructor, over the convenience of both.
It's not one I feel incredibly strongly on though - if you particularly disagree here. 🙂
There was a problem hiding this comment.
I don't have a strong preference in the abstract, but when I tried out each of the two alternatives separately, I was forced to write ugly code in one place or another. Runners and project loaders would usually have some kind of dynamic list and would be forced to construct an array from it. OTOH the 39 uses of the params constructor would have to add a new [] construct to their code.
src/NUnitEngine/nunit.engine.tests/Services/Fakes/FakeProjectService.cs
Outdated
Show resolved
Hide resolved
|
I guess the alternative to |
Interested in what anyone on @nunit/engine-team would think on this one! ☝️ I think we're into preference territory here - and it's a case where mine and your preferences differ, Charlie! 😄 |
|
I've gone with the "AddSubPackage" suggestion in place of "Named". |
|
Ready for re-review |
|
Looks good, thanks! |
Fixes #889
This fix enables us to move forward without having to support multiple constructors for
TestPackage, simplifying tests and new feature development. It's not intended as the last word on the interface provided byTestPackage, which we will need to rethink somewhat for the design of the final 4.0 release. This is covered in issues #885 and #919.The old string constructor created a package with a file name but no subpackages. The list constructor created a package with no file name and subpackages. Named packages are only used internally by the engine, so that constructor has now been replaced by the
Named()modifier. Addition of aparamsoverload makes the TestPackage constructor work the same whether one or multiple test files are used.