-
Notifications
You must be signed in to change notification settings - Fork 152
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
Fix paths in addins file for NUnit.ConsoleRunner.NetCore nuget package #1454
Conversation
@k15tfu It's not immediately clear to me what you are fixing here. Can you explain either in the description of the PR or in a bug report? ALSO... what version are you finding any error in? The current build has changed substantially since the last release. |
../../../../NUnit.Extension.*/**/tools/*/ # nuget v2 layout | ||
../../../../../NUnit.Extension.*/**/tools/*/ # nuget v3 layout | ||
../../../../nunit.extension.*/**/tools/*/ # nuget v2 layout | ||
../../../../../nunit.extension.*/**/tools/*/ # nuget v3 layout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the extra two layers of ..
?
@CharliePoole Hi, thank you for the quick reply! After upgrading to NUnit.ConsoleRunner.NetCore v3.18.1 it no longer loads NUnit.Extension.TeamCityEventListener. As far as I can see
Is it okay to add a new |
@k15tfu I did 3.18.0 because of an error in 3.18.0 making it difficult or impossible to install the .NET core runner as a .NET tool. Running as a .NET tool is it's primary purpose, so this was treated as a critical error. I think that keeping the extra level of directory in the package (net6.0) was probably a mistake and it may be better to simply have the executable in the root as it is for the standard console runner. In fact, the .NET Core runner had not been able to access extensions in the past and 3.18.1 does not test for extensibility. This is one of a number of inconsistencies between the two runners, which I would like to resolve. For the moment, I won't merge this, but I'll take some time over the weekend to think about the alternatives and incorporate this PR or a changed version into the master for the next release. I'm not sure if this calls for a 3.18.2 release or if we will wait for 3.19 but in any case the fix needs to be accompanied by tests, which actually exercise the extensions. |
@CharliePoole Please tell me if there is anything I can help you with. |
This is an area w h ere we could definitely use some help. I'm in the process of creating issues to define where the tool needs to be improved and I'll include you in them. |
Related to #1418. If we had tests to show we can load each extension then the problem would not arise. |
Closing this without merging. It's made obsolete by PR #1506, which eliminates addins files and also implements the additional levels requested here. |
cc @CharliePoole