Added searchdirectories to the navigation #143 #147

[OsirisTerje]

Bugfix for the #143 and #147 issues on finding the correct assemblies when running inherited tests in other projects.

Finds the assembly paths of those involved, and adds them to the navigation resolvers search-directories.

[CharliePoole]

@OsirisTerje I'm confused by the versioning. The Cake script has been updated to reflect version 2.1.2, which would seem to imply a hotfix release but neither of the bugs is critical. OTOH, the vsix indicates it is version 2.1.1.1.

I think we would like to move to an approach where each project is able to follow it's own scheme for versioning, different from the core projects. But, to my mind, this should only be (1) as made necessary by the nature of the project and (2) with transparency to both team members and users who can understand what policies are being implemented.

The current approach for most of our projects is to only do a hotfix release when there is a critical bug. Being "critical" is a judgement call, but the criterion is "so bad that it forces an immediate release." Low, normal and high-priority bugs get fixed but not released until the next scheduled release and we view hotfixes as the exception.

That said, if you were to take responsibility for this project, then I would be ok with any reasonable approach you laid out. In particular, since it's a simpler project than framework or engine, you might want to do bug fix releases as the fix became available. My only reservation is that I think you should lay out the approach and get the core team's concurrence. Most likely, we should better continue this discussion among the core team members.

[CharliePoole]

I'm not seeing any tests being added here. To the extent that we only have unit-level tests, this is logical but it's another point toward needing a higher-level test project eventually.

[OsirisTerje]

@CharliePoole I just wanted to get this out to the two reporters for checking, so the version numbers don't indicate anything now really. The nuget package has a pre-release tag, but as we discussed earlier, for the vsix we don't have that. I just incremented them as little as I could, so as not to indicate anything. It should really just have been like an incremented build number from a continuous feed. I have not cleaned it up at all, that's why I have marked the pull request as "don't merge" (as you explained earlier, and which I think is a good way). I would have preferred only to put up a pre-release in the nugte form, but I have seen some of the reporters lately have preferred the vsix, so I added that.
That said, we need to have a way of increment releases for this purposes. You mentioned earlier that we have a build-feed, but do we have that for the adapters as well? And if so, how is that versioned?

About the critically of this bug, I put it on High, and considered the Critical too, I am uncertain there, it depends on how many are hit by this. I know that the practice of inheriting tests from different assemblies are not uncommon, and these 2 bugs (which are the same cause, afaic see) may therefore affect pretty many. And again, I checked the download count for 2.X and it is still high, so a large user base are still on that version. Your views here would be great.
Also, should we add this to the 2.2 milestone, or push it out earlier ?

About the tests for these, I just did this as a quick fix based on @initram findings. I have also been through the Mono.Cecil code to check out what it is really doing. I am not sure we should test Mono.Cecil, and I am not sure how we should go about this at all. We could add another mock assembly to check for this case, but.... That we need some higher-level tests are pretty clear. I am also uncomfortable seeing these Mono.Cecil bugs popping up, as we really didn't intend this to be anything else than a VS2017 upgrade. I am also confused by these things not popping up in the NUnit3 adapter. The same code is there, but it doesn't behave the same way. Is that because NUnit3 already have Mono.Cecil included? Or what is going on here?

[OsirisTerje]

@CharliePoole

1. Show we set this as high or critical? If critical, should we then push it out rather soon?
2. Should we fix up tests before we proceed?
3. What are your thoughts on this working in NUnit3 and not in NUnit2 when the code is the same? Is it due to NUnit3 also using Mono.Cecil?
4. Does the change make sense to you? (Since you know this piece of code better)
5. The way I increment version numbers, is it still "not the way we do it" ?
6. Do we have a pre-release feed for the adapters? If not (which is what I have understood, should, and could we set up one?) And if so, how should we do the versioning of these pre-releases? And since nuget packages are easy to pre-release but vsix'es not, should we only do one, or should we use the vsix 4th number for that purpose?

[CharliePoole]

@OsirisTerje Answering as best I can...

1. Our definition of "Critical" already implies it needs to be put out immediately, whereas "High" or lower means it can wait a few months for a scheduled release. Of course, many things are "critical" from the point of view of the user who runs into them, so we have to decide what we think. My original comment was due to the fact that you categorized it as merely high but indicated (somewhere I can't find) that it was to be a hotfix - I was just seeing an inconsistency.

As to whether it really is critical, it doesn't seem so to me. It only comes up when you are inheriting test fixtures from a separate assembly, which doesn't strike me as a terribly common thing to do. However, if it was something we just broke it in a release we did, then I'd call it critical. Did we? If so, we can either move forward to fix it (if it's quick) or back out what broke it... I know that the last release contained a few enhancements we might be able to do without.

2. I don't see how you can add tests of this in time for a critical hotfix. They would have to be more than unit tests and we have not even created a framework for system-level tests of the adapter - which is what I was picturing. I guess your approach of having users try it out is our test for now. I do think we should actually do something about creating a framework for running high-level tests of things like this, perhaps as another github project, perhaps as a separate solution in the existing repo. I think that's more long-term however.

3. Yes, I think the problem is at least partially due to the fact that the NUnit3 engine loads and initializes the Cecil package before the adapter ever comes to use it. It can also be a difference in version - if there is one and in where the Cecil assemblies are located at execution time. When I was starting to work the issue, I had asked the user some questions about the assembly content but didn't get a full answer. Once you created a PR, I figured you had all the necessary info and didn't follow up further.

4. It's a bit of a puzzle, actually. The user indicates that the two assemblies are located in the same directory. Without your change, Cecil was apparently able to find one of them but not the other! I see that your change makes things work, but if I were doing it I would be bothered by not understanding why it works. Have you ever seen a complete layout of where everything is on disk at the time of execution?

5. Well, we've known for a long time you do it differently, but that difference has never been documented anywhere (to my knowledge) and I don't understand exactly what it is other than that you make use of the fourth component of the version for some purposes. In this particular PR, the most worrisome thing was the use of two completely different version numbers, one in the build script and one in the vsix package definition. Changing the version when not doing a release is also different - we normally don't do that but simply let the build system add a suffix.

Based on what I see, it kind of appears that you make every change look like a new release in some way. As I mentioned in another note, it's OK with me if the adapter needs to be different, I'd just like to understand it and have others understand it. "The way we do it" isn't sacrosanct, but it's the only way that anyone has bothered to write down so far.

6. Both adapters create artifacts on Appveyor every time they are built. That includes the nuget package, vsix and zip file. In addition, the NUnit 3 adapter deploys nuget packages to our MyGet feed. This is only done for changes after they are approved and merged to master so those packages are easier to use since they have been more thoroughly vetted. You an just take the latest. For Appveyor artifacts, you have to know exactly which build you want.

Adding nuget package deployment to MyGet for the NUnit 2 adapter would be trivial. Adding vsix deployment is possible as well and probably also trivial - I just have not looked too closely at it.

---

Hope this all helps you!

[OsirisTerje]

@CharliePoole About the versioning : On the appveyor feed, the vsix is named as a pre-release package like it was a nuget package, but that doesnt apply to vsix'es, so internally it keeps the manifest version number. Since vsix'es can't have pre-release numbers, the only way I see we can match it up is to use the 4th digit for that purpose. We need a convention to follow here.
About updating the version to 2.1.2 "indicating a hotfix", I seem to remember you said earlier we should upgrade version numbers only on master branch, after merging, as you state above, but that means the build system just adds it as a pre-release to the existing released build, and that then looks as a predecessor more than a successor. Or am I missing something here?

[OsirisTerje]

About 4: I am really puzzled here too, because in my own repro solution I have two projects, where one references the other, and in the main project's output all the assemblies are present. It looks like this:

Based on the reporters information, and @intgram's findings, it seems to me that Mono.Cecil looks in the typedef and extract the location from there, which would in my case be the folder of BaseTest. Since that is what is added, it can be the only reason. I would like to say this is a bug or a shortcoming in Mono.Cecil, but the workaround seems to be to add that information as a search directory.

[CharliePoole]

The versioning issue I saw in the pr is that you have 2.1.2 in one place and 2.1.1.1 in another.

In addition, there is the question of what versions we keep on the master branch. For framework and console, the third and fourth components are always zero. We advance the second after a minor release, for example to 3.7 after releasing 3.6. All the drops from that point will be pre-releases of 3.7, usually using -ci- or -dev- suffixes. We only ever change the third component on a release branch and never merge that specific change back to master.

So long as we have such a thing as hotfixes, where we release critical updates without releasing all the prior merges into master, I think we have to use release branches rather than releasing from master. I have been thinking about doing a simpler, linear approach where we always just release everything, but not everyone agrees with this.

For the adapter, there is room to do it differently if we can at least all know how it is done and/or can look it up when we need to.

[OsirisTerje]

The closer we get the adapters to the framework and console, the better. My only concern is the vsix, but we could use the 4th digit as a running kind of build number, that would leave the 3rd for a possible hotfix. I couldn't find anything on versioning in the docs, at least not from the top level, so if it is not there already, we should add the info there, both the general case, and vsix variation around the adapters. I'll update the versions as close as I can get them in the PR. If the 3 other numbers should match, then it means a release of a vsix will look like 2.2.0.34567, when a nuget adapter would only be like 2.2.0

[CharliePoole]

I'll find and point you to the references today when I'm back to my computer.

Meanwhile, it would help if you can explain the fourth component issue. Are you talking about package or assembly versions? File names?

I maintained the adapter for many years without this being an issue. As far as I understand, the only limitation was in the gallery, where we never did pre-releases.

[OsirisTerje]

The 4th component issue is for the vsix, which is what goes on the visual studio gallery, yes. We never do pre-releases there, but when we need to give out a fix-version for verification, like I did the other day for the 2.X adapter, it must have a version number higher than the previous released version, and guaranteed lower than the next upcoming version. As in this case, the next upcoming version would be 2.2.0 (or 2.2.0.0 for 4 components), so I can't use the 2.2.0.0 version number for the fix, and then I am left with using the 2.1.* series. If we never give out vsix fix versions for verification, the problem goes away, but as can be seen from the reported 2 issues, they preferred having a vsix. And, we have had issues which only affected the vsix too, so the need is there to have a convention for how to handle this.

[CharliePoole]

I'm still not 100% convinced because I've been working without using the fourth component since I wrote the first version of the vs adapter. When I have installed a vsix by double-clicking on a file it has always worked fine for me. Isn't this the same situation you are talking about? Does it stop working with some particular version of VS? Note that I always uninstall any earlier vsix first. In recent years, I've been pointing people to the AppVeyor NUnit 3 artifacts when something needed to be tried out. This has seemed to work for them.

All that said, I see no problem with an approach that uses an internal vsix version with a varying fourth component whenever needed. Where I do see a problem is if we have to change the version manually every time we build. If we only want it to advance when we do an AppVeyor build, then that can easily be incorporated in the script. If we need it to advance at other times, we might need to think of a different approach.

[OsirisTerje]

When we sort of publish a version, either on appveyor, or like I did by just attaching it to an issue, it should have a unique version identifier, otherwise there is nothing that separates that version from the previous one.
In addition, earlier I have had issues with the adapter where it either wasn't properly uninstalled, or I simply needed to know for sure I had the new version in, so the only way to fix these things is to update the assembly file version (because that is what is shown in the Output/Test window), and again, I then update the vsix 4th number to make sure I know which version I am currently working with.
Instead of attaching a manual version to a comment, I could just as well used the appveyor artefact link and pointed to that. So wrt to publishing fixes using the appveyor feed is good, and a script change to autoupdate the vsix would be nice. Locally I think it doesn't really matter what one do, as long as the version change is not committed and pushed up.

[OsirisTerje]

Also, would be nice to add the nuget package for the adapter to the MyGet feed. I added an issue #152 for that and assigned to you :-)

[OsirisTerje]

No further comments after I updated the version numbers, so I'll merge this now.