nunit3-console loads nunit.framework with partial name

[cyanite]

This prevents running tests when there are non-trivial assembly codebase entries in the app.config file, since redirection polices are not applied when loading with partial name. In general, you should never load with partial name.

The bug is in the NUnit3 driver and the fix is not difficult, since the full assembly information is already available for a load with full name. I'll create a pull request for it ASAP.

A similar issue exists in NUnit 2 and its driver for NUnit 3. I'll create a separate issue for it.

[CharliePoole]

Looking at the fix in your fork, this won't work. The NUnit engine is supposed to work with any version of the framework. That's why the engine cannot specify the version to be loaded.

In normal use, the driver already finds the framework assembly in memory because it has already been loaded as a side effect of loading the test assembly. That's why the partial name works in this case. Test frameworks tend to have bits like this, which violate the normal "rules" of coding in order to make things work. We want to keep those violations to a minimum, of course.

WRT NUnit V2, the intent is the same although the implementation is different. Their is no actual reference from nunit.core to nunit.framework because a given installation of NUnit 2.x is supposed to run tests built any framework >= 2.0 and <= 2.x.

Best way to proceed with this issue is to give us an actual use case where NUnit fails to run the tests.

[cyanite]

An example of the problem solved: If I have a test assembly, foo.dll, and a config file, foo.dll.config:

<configuration>
  <runtime>
    <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
      <dependentAssembly>
        <assemblyIdentity name="nunit.framework" publicKeyToken="2638cd05610744eb" />
        <codeBase version="3.9.0.0" href="nuget\nunit.framework.dll" />
      </dependentAssembly>
    </assemblyBinding>
  </runtime>
</configuration>

And I then run nunit3-console foo.dll, it will fail because it tries to load nunit.framework from the current directory (and the nunit3-console directory), not from the correct location. Loading with full name works correctly.

[cyanite]

Looking at the fix in your fork, this won't work. The NUnit engine is supposed to work with any version of the framework. That's why the engine cannot specify the version to be loaded.

Yes I know. My fix doesn't change that. The assembly name I load is reflected from the actual test assembly.

In normal use, the driver already finds the framework assembly in memory because it has already been loaded as a side effect of loading the test assembly. That's why the partial name works in this case. Test frameworks tend to have bits like this, which violate the normal "rules" of coding in order to make things work. We want to keep those violations to a minimum, of course.

Yes, but in this case it's loading the framework in order to figure out which driver to use.

WRT NUnit V2, the intent is the same although the implementation is different. Their is no actual reference from nunit.core to nunit.framework because a given installation of NUnit 2.x is supposed to run tests built any framework >= 2.0 and <= 2.x.

I know, but the problem is the same and can possibly be resolved in the same way (depending on how you reflect for the nunit.framework assembly in NUnit 2).

[CharliePoole]

Regarding the example... I haven't run it but i'll take your word it fails because it makes sense that it would.

However... is there any good reason for doing this? Since NUnit is designed to work with whatever version of the framework the tests are bound to, why would you use a binding redirect?

IOW, even if this is a confirmed bug, we might say "just don't do that" if there isn't a good use case.

[CharliePoole]

"The assembly name I load is reflected from the actual test assembly."

I didn't see that, I only saw hard-coded "3.9.0". I'll check the PR again.

[CharliePoole]

All right, I see what you're doing now. I cancelled the review and will re-review. I'd still like to know why one would do a binding redirect for the framework, however. We've put a lot of effort into making that unnecessary in the past.

[cyanite]

For various reasons we have moved our external assemblies (nuget dependencies) to a separate directory. It's not just the nunit.framework. One reason is that we have a large legacy code base with some test assemblies using NUnit 2 and some NUnit 3. Doing it like this, we can run the entire test suite with NUnit 3.

[CharliePoole]

Thanks. That makes sense.

[CharliePoole]

@nunit/framework-team

The main argument for not fixing this is that we have had similar behavior for at least 15 years (I wasn't on NUnit before that) and this is the first time the problem was brought up. There must not be many people doing this and what you are doing is definitely not how NUnit was designed to be used. We have been recommending against use of binding redirects to force use of a different framework version for as long as I can remember. Add to that the fact that how we decide what driver to use is one of the most critical parts of the engine and I get worried.

The main argument for fiixing it is that it affects one user.

I would vote to accept and fix this but only if we can demonstrate with high certainty that the fix does no harm.

[cyanite]

The main argument for not fixing this is that we have had similar behavior for at least 15 years

Respectfully, I don't think that's a very good argument. At any rate, I am not really appealing to authority. I don't care if it's Microsoft or who says it or what they think, I care that it's true: That partial assembly loading is problematic because it doesn't always do what you think it does. That's how it is, that's how Fusion works, regardless of what anyone thinks. The main issue is, quoting from the document: "The Assembly.LoadWithPartialName method might load a different assembly with the same simple name." Now, we're not using LoadWithPartialNameHere, but it's the same code we're activating by invoking Load with a partial name (which we do via CreateInstanceAndUnwrap).

There must not be many people doing this

Agreed. It's not too common to use codebase redirects.

and what you are doing is definitely not how NUnit was designed to be used.

I don't see how what I am doing has anything to do with NUnit directly. NUnit should just load the assemblies I specify. Currently, it doesn't, because it doesn't load the assembly names that are actually referenced, even though it correctly pulls those references out of the test assembly.

NUnit goes to great effort to load the correct configuration (app.config) into its target domain, but then disregards that configuration by not loading assemblies into the domain correctly. The right assembly will eventually be loaded by the CLR, since it's a dependency of the test assembly and because the CLR never loads by partial name, but by then it's too late.

Add to that the fact that how we decide what driver to use is one of the most critical parts of the engine and I get worried.

Fair enough. It certainly is a change of behavior, but I can not see any situation where something that worked before will now not work. I can only see situations where it wouldn't have worked before, but now does, namely if there are redirects or code base entries in the app.config for the test assembly.

The main argument for fiixing it is that it affects one user.

Well, a strong argument for me is that it's the right thing to do. Loading by partial name in a foreign domain which configuration you don't know, really isn't.

I would vote to accept and fix this but only if we can demonstrate with high certainty that the fix does no harm.

I suppose I can enumerate and check a number of scenarios, but how would you be convinced we have covered everything? I'm very willing to work on that, though.

[CharliePoole]

I asked for a team discussion. Basically, the question for us is whether we want to support your scenario. It's a scenario that the NUnit team decided not to support years ago. You have given the reasons why you think we should support it and there's not much more you can say until other team members have actually joined the discussion.

Since my own stated conclusion was in your favor, it's a little silly of you to keep arguing with me. 😄 In fact, this last post has given me some second thoughts.

If we allow binding redirects to function, are we then committing to keeping them working in other scenarios we haven't yet envisioned? Historically, NUnit guaranteed that we would use the exact version of the framework that the user referenced from code. Allowing redirect to other, possibly incompatible versions has a few risks. We can't prevent the user from doing it, of course, but we can advise against it - as we have in the past. So, I guess I'm moving a bit away from in favor toward uncertain now. I'd go for this particular change if we are sure it won't break anything and if we make sure it doesn't imply broader support for redirection. We also have to note that it only applies to the NUnit 3 framework. Any extensions would need to make their own decision about what they support.

Let's pause a beat and give some of the @nunit/framework-team members a chance to comment. 😄

[jnm2]

I'm coming to this without a prior opinion.

There must not be many people doing this and what you are doing is definitely not how NUnit was designed to be used. We have been recommending against use of binding redirects to force use of a different framework version for as long as I can remember.

None of this is something I've read in the docs or heard before. (I'm new.) Either way, I sympathize enough with the actual use case (codebase href) that I'm willing to try to plan through the possible repercussions of enabling version redirects.

@cyanite Thank you for your willingness to help work through this.

@CharliePoole No objections come to my mind. Would you please bring me up to speed with what's at stake with version redirects of the framework? Linking past discussions from this one would be awesome.

[jnm2]

Historically, NUnit guaranteed that we would use the exact version of the framework that the user referenced from code. Allowing redirect to other, possibly incompatible versions has a few risks. We can't prevent the user from doing it, of course, but we can advise against it - as we have in the past.

I would have thought that binding redirects, whether autogenerated due to conflicts or manual, are more likely to be the source of truth for what the user actually wants to reference than what they referenced (directly or transitively).

[CharliePoole]

@jnm2 I'm afraid we have been through at least two different discussion platforms over the time since I started working on NUnit, so I can't point you to anything.

As stated, I favor doing this. I was initially put off by the lack of explanation as well as the tone of the request but that's neither here nor there. I think it makes perfect sense to make use of an assemblyname that we already have if we can do so without messing up the general design of drivers and their factories. Somebody needs to at least go through the thought experiment of trying to apply this idea to a hypothetical driver extension lfor a foreign framework like jnm.framework. Will we have all the same info for such a framework that we have for one of our own?

Ideally, I think our goal should be to use the same framework instance as is used by the test, no matter how that version was arrived at.

Of course, if we don't do this, the user can work around the problem by modifying the config for the console runner and agents.

[CharliePoole]

@jnm2 I guess it depends on who the "user" is. If the person setting up the binding redirect is knowledgeable, then they will have thought of all the issues that could arise. But if it's someone who just wants to run tests that they have not written, it can lead to silently ignoring tests, false positives and other hard to debug issues. It's a very sharp knife.

[jnm2]

Somebody needs to at least go through the thought experiment of trying to apply this idea to a hypothetical driver extension lfor a foreign framework like jnm.framework. Will we have all the same info for such a framework that we have for one of our own?

Do we have an existing example?

Ideally, I think our goal should be to use the same framework instance as is used by the test, no matter how that version was arrived at.

I would say, the test uses whatever the test project is set up to output for its binding redirects.

If someone wants to run tests that they have not written, I can't think of a scenario where they edit the binding redirects any more than changing the project references and package dependencies. The activities go hand in hand. My lack of imagination is of course not evidence. 😜

[CharliePoole]

We don't have an example other than the NUnit V2 driver.

I've seen some very strange things as a coach, including binding redirects in a special config created by QC because they wanted to use a different framework version than what the developers had used to create them. It's an odd thing to do, for sure.

One would like to provide features that can only be used well, but that's not really possible.

My opinion is that we should move ahead to implement this if you and @rprouse agree. As you know, I am trying to avoid being the deciding vote in anything these days, instead just pointing out what I think the issues are.

[cyanite]

As stated, I favor doing this. I was initially put off by the lack of explanation as well as the tone of the request

Sorry about that.

Historically, NUnit guaranteed that we would use the exact version of the framework that the user referenced from code.

But... but that's exactly what you don't do currently. My PR fixes it so you do use the exact version of the framework that the user referenced from code. The current state of affairs is that that doesn't always happen. I have posted detailed examples below :)

Allowing redirect to other, possibly incompatible versions has a few risks.

Note that none of my examples involve version redirection. They all simply involve placing the file in a different location.

Ideally, I think our goal should be to use the same framework instance as is used by the test, no matter how that version was arrived at.

Yes, which is why I made the PR.

Somebody needs to at least go through the thought experiment of trying to apply this idea to a hypothetical driver extension lfor a foreign framework like jnm.framework. Will we have all the same info for such a framework that we have for one of our own?

I should think so. As long as we detect references using Mono.Cecil or similar.

We don't have an example other than the NUnit V2 driver.

None of this is V2 driver related. In fact, the V2 driver suffers from a similar (but slightly different) problem, which I didn't yet debug. This is all V3.

[cyanite]

In the following text I list 5 cases. In 4 of those the existing nunit fails to execute tests, while the PR version works in all cases. The lines under "CASE x:" lists the files located in the directory. The test assembly is tests.dll. In cases where it says "GAC:" this assembly is installed in the GAC instead of being located in the directory.

Note that two of the cases where the exsiting version fails don't involve app.config files, although I do admit that they are slightly contrived.

Sorry for the formatting.

---

CASE 1: tests.dll (depends on nunit v3) nunit.framework.dll (v3)

EXISTING: works correctly AFTER PR: works correctly

---

CASE 2: tests.dll (depends on nunit v3) tests.dll.config (point nunit v3 -> "nunit.framework.foo.dll") nunit.framework.foo.dll (v3)

EXISTING: System.IO.FileNotFoundException : Could not load file or assembly 'nunit.framework' or one of its dependencies. The system cannot find the file specified. AFTER PR: works correctly

---

CASE 3: tests.dll (depends on nunit v3) tests.dll.config (point nunit v3 -> "nunit.framework.foo.dll") nunit.framework.foo.dll (v3) nunit.framework.dll (v2)

EXISTING: System.TypeLoadException : Could not load type 'NUnit.Framework.Api.FrameworkController' from assembly 'nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77'. AFTER PR: works correctly

---

CASE 4: tests.dll (depends on nunit v3) GAC: nunit.framework.dll (v3)

EXISTING: System.IO.FileNotFoundException : Could not load file or assembly 'nunit.framework' or one of its dependencies. The system cannot find the file specified. AFTER PR: works correctly

---

CASE 5: tests.dll (depends on nunit v3) nunit.framework.dll (v2) GAC: nunit.framework.dll (v3)

EXISTING: System.TypeLoadException : Could not load type 'NUnit.Framework.Api.FrameworkController' from assembly 'nunit.framework, Version=2.6.4.14350, Culture=neutral, PublicKeyToken=96d09a1eb7f44a77'. AFTER PR: works correctly

---

[CharliePoole]

@cyanite Thanks for the additional information. I'm marking this as confirmed but still leaving it as an "idea". I'd prefer for some other team member to concur with me and promote it out of that status. This doesn't call for any more arguments on your part, it's just a way of working we have. 😄

[ChrisMaddock]

Both - between PR and issue, there's over 50 comments here now! Would someone who's been keeping up mind summarising the latest status, which you want people's opinions on? Thanks! 😄

[CharliePoole]

@ChrisMaddock I think we should support this change, but I see issues the team should consider. As the guy with one foot out of the door I didn't think I should decide myself whether you will support this in the future.

1. Do you want to support binding redirect of the framework. In the past we said we didn't and just told people who complained not to do that.

2. We don't currently have any third-party drivers, unless you consider NUnitV2 as one. Can you see any problems that this would cause in their case?

I see some specific code issues in the PR, which I would note when I review it. Didn't want to review it unless we are going to do this.

[cyanite]

Do you want to support binding redirect of the framework. In the past we said we didn't and just told people who complained not to do that.

Not to keep commenting, but note that I am not doing binding redirects but rather codebase entries, i.e. changing the physical location of the nunit.framework.dll file. It would support binding redirect as well, but since v2 and v3 frameworks are incompatible, this wouldn't really work anyway, and I agree that there is absolutely no reason (or even possibility) to support it. Again: I am not in any way advocating binding version redirects.

We don't currently have any third-party drivers, unless you consider NUnitV2 as one. Can you see any problems that this would cause in their case?

The current code (before my PR) is hard coded to "nunit.framework", whereas I use the one passed to the method, so if anything the support should be easier.

[CharliePoole]

@cyanite You only changed the nunit 3 driver. I'm asking what this implies for other drivers as well as the driver service, whose job it is to find drivers.

Your point about binding redirect is correct.

[cyanite]

You only changed the nunit 3 driver. I'm asking what this implies for other drivers

@CharliePoole right. There is a similar problem with the NUnit 2 driver as well. The problem is actually inside the nunit.engine (v2) shipped with that. I haven't debugged it exactly, but I know (from fuslog) that it's also due to loading with partial name. It doesn't stop tests from being executed, but it prevents TestActions from being invoked, which I noticed in our test suite. The tests that didn't use actions worked fine.

Fixing this bug correctly would probably mean fixing it in the NUnit 2 nunit.engine assembly, which is why I didn't want to mix it into this issue. I'm of course willing to do that as well, but it does raise the question of how to do it since currently those files are included in binary form in the v2 driver repo.

as well as the driver service, whose job it is to find drivers.

The driver service isn't impacted. That's the one which digs up the reference to nunit.framework that I now use in the v3 driver.

[CharliePoole]

@cyanite It's always confusing for the issue poster when I do my thing and broaden it. 😄 I realize this PR doesn't impact anything but the V3 driver. What I'm asking myself and the other team-members is whether this PR will lead us to change the interfaces of the driver service and other infrastructure in the usual way that approving one change leads to needing to make another change.

If it does, that doesn't mean we won't approve your change, it will just affect where we go next with the re-design of those interfaces, which has already had some discussion in the past.

It's really too bad we never created a simple driver extension before. Both the NUnit drivers are handled in slightly exceptional ways: NUnit 3 is built in and NUnit V2 has a special extension point just for its use. I may end up creating such an example just to show how it works.

Apologies that you are suffering delay due to our organizational changes. I don't want to saddle the team with something that gives them extra work as I leave. Otherwise, I might have approved this already.

[cyanite]

It's always confusing for the issue poster when I do my thing and broaden it. 😄 I realize this PR doesn't impact anything but the V3 driver.

@CharliePoole ah, I see now, sorry for the misunderstanding :).

Apologies that you are suffering delay due to our organizational changes. I don't want to saddle the team with something that gives them extra work as I leave. Otherwise, I might have approved this already.

No sweat. Since we (will soon) have a mixed test suite (v2 and v3) and we have a deadline moving up, we'll have to run a custom NUnit build to begin with anyway, since there is still the v2 driver similar bug I spoke of above (which can be circumvented rather easily, though). Let me know if/when there is anything I can help with.

[rprouse]

I am not against the idea although I do see minor issues with the code. I don't think the constructor of the driver is part of our API, just the interface it exposes. It is the factory's responsibility to create the driver. I think that @cyanite's use case for using a binding redirect is fairly edge case, but it is valid.

@CharliePoole can you explain how you see us possibly having to extend or change the driver interface? Is it because of how the v2 driver works? As I see this PR, all change is between the driver and its factory which are a pair.

It is interesting that nobody has even asked about creating another driver. I've always wanted to mock one up for xUnit or MSTest, but never find the time 😄

Unless anyone else on the @nunit/framework-team has issues, I am okay with moving on to the PR and reviewing it there. I do see some technical details I would like to see changed.

[CharliePoole]

@rprouse Nothing specific except that we had talked about changing the IDriverFactory interface. With this change, we will definitely have to retain the method that uses an AssemblyName, even if we add other methods. I don't see that as a problem. It's just my ongoing hesitancy to accept anything that might lead to an API change without asking for your input that's in play here.

I'll move this from idea to bug on the basis that neither @rprouse nor @jnm2 nor @ChrisMaddock have raised objections.

[CharliePoole]

@cyanite I may have misunderstood an earlier comment about your conversion from V2 to V3. I thought you meant you were using a binding redirect to force V2 tests to run under V3. I think we can support both codebase changes and binding redirects with the understanding that it's up to the user to determine whether the assembly being directed to actually works.

WRT V2 - we no longer update the V2 code and we use the binaries from 2.6.4 as part of the V2 driver. I have an incipient project to re-introduce V2 support, independent of the NUnit Project. (See code at https://github.com/CharliePoole/nunitv2)

If that goes to release then it is possible to release a nuget package of core+util and use it in the V2 framework driver. We'll see. This is an NUnit governance issue as well, so it may take a little time.

[cyanite]

I may have misunderstood an earlier comment about your conversion from V2 to V3. I thought you meant you were using a binding redirect to force V2 tests to run under V3.

No, that won't work due to incompatibility between v2 and v3. Our running (and for test) system consists of 100s of assemblies, and what we'll do is gradually switch test assemblies over to NUnit 3, while keeping many on NUnit 2. Since there can't be two files called "nunit.framework.dll" in the directory, obviously, we decided to rename them both, and since we are doing that anyway, we're moving all NuGet dependencies into a separate directory and tag them with their NuGet version numbers (we have a custom build system). Then we add codebase entries for all of them, to direct assemblies to correct files.

I think we can support both codebase changes and binding redirects with the understanding that it's up to the user to determine whether the assembly being directed to actually works.

Yes, I agree. The PR will make that possible, and it might work between minor versions, so it might be useful for some people.

WRT V2 - we no longer update the V2 code and we use the binaries from 2.6.4 as part of the V2 driver. I have an incipient project to re-introduce V2 support, independent of the NUnit Project. (See code at https://github.com/CharliePoole/nunitv2)

Right, ok. As a work around, you could ship the v2 driver with a v2 nunit.framework.dll in the same directory. This will take care of cases 2, 3, 4 and 5 above, since loading by partial name also looks in the loading assembly (or entry assembly, possibly, not sure)'s directory. This is the work around I mentioned earlier. Now, it will still fail if it finds a nunit.framework v3 first, i.e. if that one is located in the test assembly directory, but it's better than nothing. (It will work for us, since it won't find nunit 2 nor 3 frameworks in the test assembly directory, as we moved them into nuget/).

If that goes to release then it is possible to release a nuget package of core+util and use it in the V2 framework driver. We'll see. This is an NUnit governance issue as well, so it may take a little time.

Right, that would of course be the best solution :).

[CharliePoole]

While we could get away with it due to the sunsetting of NUnit V2, that idea does break the notion that the runner code is independent of the version of the framework that one uses. We might be able to do a package dependency, however.

In any case, whatever we do presumes we are shipping a new version. 😄

[cyanite]

While we could get away with it due to the sunsetting of NUnit V2, that idea does break the notion that the runner code is independent of the version of the framework that one uses.

As for the work around: the v2 nunit.framework.dll file would just reside in the v2 driver directory next to v2 nunit.engine etc., and would normally never be loaded. Only in the cases above it would be loaded... well not even loaded, because load by partial name works like this: After opening the file it extracts the full assembly name, reapplies app.config policies and tries a normal load which will then succeed.

It will basically just make sure loading succeeds in some situations where it would normally fail. No situation that works now would stop working since the file would never be found (since it looks in the test assembly dir first). TL;DR: I don't really think it would break the notion of independence, except maybe in spirit

In any case, whatever we do presumes we are shipping a new version. 😄

Well, the work around only requires a new version of the nunit 3 v2 driver. Well.. maybe I should create a separate issue for it now. This thread is long enough as it is :)

[cyanite]

I mentioned this in the pull request, but repeating it here. I have a small repro for the issue: https://github.com/cyanite/nunit3-issue367-repro

Building that and running nunit3-console.exe on the produced assembly reproduces the issue.