Skip to content
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 loading of extensions referencing Engine API v3.0.0 #813

Closed
wants to merge 1 commit into from

Conversation

ChrisMaddock
Copy link
Member

Fixes #807. This is a pretty unsatisfying fix to the problem. It feels like we're going down a rabbit hole, but I'm struggling to come up with a cleaner solution.

I'd appreciate some feedback on the approach, or any ideas of other solutions.

Root cause

Since Engine version 3.0, the nunit.engine.api.dll assembly has not had the version number incremented. We've intended to only make what we thought were backwards-compatible changes to this assembly, however we've recently found that these changes have actually been causing a number of issues: ref: #6, #424, #800.

There seems to be two viable solutions to this problem: 1) Stop making any changes to nunit.engine.api.dll or 2) start versioning the assembly when changes are made. I personally consider option 1 to be too restrictive to future development.

For that reason, we started versioning nunit.engine.api.dll in v3.12-beta1.

Extension loading issue

Issue #807 highlighted that changing the version number of nunit.engine.api.dll had broken extension loading, for the NUnit-Org packaged extensions. The reason for this is that these extensions reference nunit.engine.api v3.0, but are not packaged with that assembly. Previously, this hadn't caused a problem as api v3.0 was always supplied with the engine, however with the above change this is no longer the case.

Workaround-fix

This PR works around the above problem by forcing nunit.engine.api.dll v3.0 to always resolve to the same version as the engine, which allows the existing extensions to still be loaded.

I think this is acceptable as it's restricted to only apply to nunit.engine.api v3.0, which will be the only version affected by this issue, once we start incrementing the assembly version. It should also only affect cases where the engine is loading the API assembly itself, which as far as I can think, would only be in an extension loading scenario.

Interim "proper" fix

Extensions should start packaging their referenced version of nunit.engine.api.dll alongside the extension assembly.

v4 fix

For v4, I think we should split the extension API off from the main API assembly used for controlling the engine. The former is stable and sees a very limited number of changes, whereas the latter we wish to change more regularly. This is touched on in #770. Extensions should also continue to package the appropriate API version.


Would appreciate all thoughts on this - I've not found it a nice problem to reason about...

@CharliePoole
Copy link
Member

@ChrisMaddock I share your reservations about using this as a fix. For one thing, I'm not sure what will happen once a single extension is upgraded to use the new API assembly. Are we locking in extensions to only work with a specific engine version?

@ChrisMaddock
Copy link
Member Author

Are we locking in extensions to only work with a specific engine version?

This is exactly what I'm trying to avoid doing here. But I'm also not an expert on the complexities of .NET assembly loading logic, so still concerned this may cause unintended side effects.


I'm considering delaying this change until we can break up the API in v4.0, and instead putting a freeze on changes to the nunit.engine.api assembly for the remainder of 3.x. That sounds like it will really impact development and move us further away from our desired architecture, but I'm struggling to think of an alternative solution to safety give us stability here.

I think the key issue is that modifying a production assembly while keeping the same version number works against the principles of .NET's build system, and continuing to do that will keep leading to unexpected and tricky to diagnose problems. There's all sorts of arguments as to whether the adapter should be copying to bin, and whether ReSharper should be loading assemblies from bin - but I think ultimately versioning the API assembly in the normal way would have avoided all these issues.I do think that's something we need to work out how we can get away from in 4.0. 😞

@ChrisMaddock
Copy link
Member Author

I'm going to close this PR for now, see #826 for the reasoning.

@rprouse rprouse deleted the issue-807 branch April 5, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Could not load file or assembly 'nunit.engine.api, Version=3.0.0.0,
2 participants