Fix --list-extensions showing wrong target framework for extensions - #1861
Conversation
DisplayExtension() never showed a target framework at all, and the only existing per-extension framework computation (ExtensionAssembly.FrameworkName) lives behind the local NUnit.Extensibility.IExtensionNode interface, which the console only ever sees through the older, externally pinned NUnit.Engine.Api/NUnit.Extensibility.Api package version consumed transitively by nunit.engine -- so extending that interface doesn't reach ConsoleRunner without a coordinated package version bump across the repo. Read the extension's declared target framework straight from its own assembly metadata (TestCentric.Metadata, same library and technique already used by ExtensionAssembly) instead, so the console reports what the extension assembly itself declares rather than the runtime it happens to be loaded under. Fixes nunit#839
|
@HarnageaGabriel I approved your PR to run so we can see how it works. I'd actually prefer a unit test as well, but lets hold off on that. I agree with what you stated about the "natural fix", which is what I would really prefer...
I think we could make this work but let's see how the present fix works first. |
|
@HarnageaGabriel Your build has run afoul of ongoing changes to the build process. The recipe used by all the repos I manage has been updated, but not all the repos have been updated to work with it. I'll get a change in so it builds correctly and then retry your PR. |
|
@HarnageaGabriel I just pushed a new version of the cake recipe, which may help. Change your build.cake to use version 2.0.0-beta.4.9 of the recipe. No guarantees, I'm afraid, since you are doing something new by accessing the extensibility framework from within the console runner. It's exactly the purpose for which I made NUnit.Extensibility a separate assembly, but it hasn't been tried out yet. If that doesn't work, I'll pull your changes down and play with the code myself. |
|
Thanks for the pointer — bumped build.cake to NUnit.Cake.Recipe 2.0.0-beta.4.9 and pushed. Let me know if it needs another retry or if you'd rather pull the branch down yourself. |
CharliePoole
left a comment
CharliePoole
left a comment
There was a problem hiding this comment.
This works great now that the build is fixed. We can merge it or work for a better solution involving the extensibility API. Or we can do both. See general discussion for more about this choice.
| _outWriter.WriteLine(ColorStyle.Value, node.AssemblyVersion.ToString()); | ||
|
|
||
| _outWriter.Write(INDENT8 + "Framework: "); | ||
| _outWriter.WriteLine(ColorStyle.Value, GetTargetFrameworkDisplayName(node.AssemblyPath)); |
There was a problem hiding this comment.
In an ideal solution, this would simply use something like node.TargetFrameworkName and method GetTargetFrameworkDisplayName wouldn't be needed. OTOH, we could deploy this immediately and its availability would assist in debugging problems we tend to run into with loading extensions.
|
This is all passing with the use of the (interim) recipe version 2.0.0-beta.4.9. Nice work! Theoretically speaking, we have three possible paths here.
Not merging what we have seems silly to me so I discount 2 and I'll go ahead and merge this. So the question remains... would you like to work on the "improved solution" going forward? You pointed out some of the difficulties with the API in your analysis, but I think they are all soluble. Equally important, they are difficulties we will eventually need to solve anyway, so why not now? Let me know and we can start discussing the next steps. Charlie @OsirisTerje I wanted you to take a look at this issue because the solution @HarnageaGabriel provided is the same sort of thing that the adapter may need to do at some point - not for the same reason, but for some as yet unknown problem with extensions. |
|
@CharliePoole I'm interested in taking a crack at the improved solution — threading TargetFramework through IExtensionNode properly. Could you point me at the repo where NUnit.Extensibility.Api actually lives/gets published from? Want to understand the versioning/release process before proposing changes there, and happy to loop in @OsirisTerje if there's overlap with the adapter's needs. No fixed timeline on my end, just want to get the scope right. |
|
@HarnageaGabriel That's great. For a fuller discussion of the overall process, we should probably get out of this thread. :-) Send me an email at charliepoole AT gmail DOT com and I'll respond with more info. As to the repo... it's this one! look at As for the adapter, that's a bit further in the future, but I can imagine it needing to use the extensibility interface directly rather than through the engine, so I wanted to point this issue out. |
2 participants
Fixes #839
Problem
--list-extensionsnever displayed a target framework for installed extensions. The user-visible symptom reported in #839 was that a TeamCity extension targeting .NET Standard 2.0 appeared to be running under .NET 4.0/4.7 — the runtime the console itself was running on, not the extension's own declared target.Root cause
ExtensionAssembly(src/NUnitCommon/nunit.extensibility/ExtensionAssembly.cs) already reads an extension assembly's ownTargetFrameworkAttributecorrectly viaTestCentric.Metadata, exposed asFrameworkName. But that value was never surfaced anywhere —ConsoleRunner.DisplayExtension(src/NUnitConsole/nunit4-console/ConsoleRunner.cs) never printed a framework field at all.The natural fix would be adding
TargetFrameworktoNUnit.Extensibility.IExtensionNodeand threading it throughExtensionNode. However,nunit4-consoleonly ever sees extension nodes through the console'sIExtensionService/IExtensionNode, which resolve (transitively, throughnunit.engine'sNUnit.Engine.Apipackage reference) to the externally published, version-pinnedNUnit.Extensibility.ApiNuGet package rather than this repo's local project source. Extending the local interface doesn't reach the console without also bumping and republishing that package — out of scope for this fix.Fix
ConsoleRunnernow reads each listed extension's declared target framework directly from its own assembly file (node.AssemblyPath), using the sameTestCentric.Metadatatechnique already used byExtensionAssembly. This reports what the extension itself declares, independent of the console's own runtime.Note: the issue also speculated about a secondary bug where an extension targeting .NET 4.7 might display as .NET 4.0 — that would be in
TestCentric.Metadata's own attribute/runtime-version parsing (an external dependency), not in this repo, so it isn't addressed here.Testing
dotnet buildon the affected projects (0 warnings, 0 errors).net462andnetstandard2.0builds of the repo's ownFakeExtensionstest data that the new code path correctly reports.NETFramework,Version=v4.6.2vs.NETStandard,Version=v2.0for each assembly's own target, rather than the console's runtime.--extensionDirectoryto pick up the looseFakeExtensionsbuild locally to screenshot full CLI output (addin discovery/packaging format), so verification was done at the metadata-reading level directly.dotnet testfornunit4-console.testscurrently fails to run in this environment due to a pre-existing, unrelated NUnit3TestAdapter/nunit.engine.api v3.0.0.0binding issue (reproduces identically onmainwithout this change).