Proposed solution to Issue-231 #309
Conversation
…led (maybe because of different NUnit versjons in dll's)
| { | ||
| var msgNode = loadResult.SelectSingleNode("properties/property[@name='_SKIPREASON']"); | ||
| if (msgNode != null && (new[] { "contains no tests", "Has no TestFixtures" }).Any(msgNode.GetAttribute("value").Contains)) | ||
| TestLog.Info("Assembly " + assemblyName + " contains no tests supported by this runner: " + _activeRunner.GetType().AssemblyQualifiedName); |
There was a problem hiding this comment.
I'm not sure what the user will understand by "no tests supported by this runner." Is there another runner that the user should use? What do we mean by a runner? etc.
I think the fix is good if we can come up with a meaningful message.
There was a problem hiding this comment.
Yeah, well - I don't know what that should be. I stumbled across this issue when one of the projects in my solution was linked with an older version of NUnit, I have no idea what other situations could cause this.
Of course I can change the text runner with TestRunner, but that doesn't help much.
There was a problem hiding this comment.
What about: "contains no tests supported by NUnit3" . It is NUnit3 that gives this message, so the adapter should only report it.
There was a problem hiding this comment.
Maybe, but it is not quite correct. In my case it was a mismatch between 3.4 and 3.5 that caused the fault (thats why I returned the full AssemblyQualifiedName in the message)
There was a problem hiding this comment.
It might not be absolutely correct, but I think the message should be directed toward the general user, and inform about possibly user mistakes. If there is a NUnitAdapter/NUnit mismatch issue that is possibly a bug in the adapter, as any adapter version should ideally be able to run any NUnit version (and I know it is not really true this will happen). But I am not sure we need to surface that in this message. Alternatively, replace "this" with the explicit name and version we're running. "contains no NUnit3 tests that can be run by NUnitAdapter version xxx". But, again, the message comes from reading the NUnit3 skipreason node, so the adapter should be innocent here.
There was a problem hiding this comment.
Okay, so the message should be along the lines of "NUnit3 was able to discover some tests in {assembly}, but could not run thoose tests. Please inspect your projects assembly dependencies. (TestRunner was: {AssemblyQualifiedName})" ?
There was a problem hiding this comment.
Yeah, that looks good to me :-)
|
That's the sort of confusion I was worried about. The user may think that "runner" is the adapter. Or maybe the engine and one of its internal runners. But we know it's the nunit 3 framework that was used and failed to find the test, so let's say that. The actual reason it failed to find the test is another matter but at least the problem should be reported to the right project. |
|
I still have question. How do we know it originally discovered tests but now fails to find them? Do we just assume it's so |
|
The code is in the test executor, so I would assume it is safe to assume that they were discovered first, and then attempted to be run, but failed. |
|
@OsirisTerje I agree that that sounds logical. However, how and when VS calls us is sufficiently obscure and undocumented, that it might be worthy running some experiments to figure out what the rules are. For example, are we ever called for execution when we returned no tests from discovery? Does this vary between the IDE, vsconsole, TFS and (eventually) dotnet test? We have never been able to get solid info about this from Microsoft. In part this is probably due to the fact that various groups there work on the code and they are somewhat compartmentalized. We, on the other hand, have a need to see the bigger picture in order to integrate with their software. I'm thinking we should do the best we can to come up with a reasonable message, merge this PR and close the issue. Following that, perhaps we should create a new issue to experimentally determine for ourselves exactly how and when we are called in each environment. I'm also concerned that whatever we do here may become broken when we upgrade to use the newest engine. So as yet another issue, maybe we need to set up a build of the adapter that uses the latest development version of the engine. what do you think? |
|
@CharliePoole Yes, good points. With the possibility for being broken later, I assume you mean the check against a certain string value., which of cause is brittle. But since this is XML parsing, are there any other good options? Agree with the message though, so I would just suggest it states what it know, and don't assume anything. What we know is that we're trying to execute NUnit3 tests and that failed. If discovery found them, that is visible in the output log too. I fully agree we need to set up some builds to check the different issues and possible scenarios, but also keeping these builds for later checks (like what I also experienced now with the 2.X adapter and the 3.5 issue.). There are too many variations now, so the risk of breaking something is just getting bigger. |
|
@OsirisTerje Feel free to merge when you think the message is right and we can deal separately with this other stuff. @pi3k14 Including the name of the runner in your message doesn't make sense as it is constant. I think we just want to say that NUnit (i.e. the nunit framework, of whatever version you are using) didn't find any tests. I realize it's only a small improvement on the existing message, but that's all we actually know for sure at that point in the code. |
|
@OsirisTerje I was thinking of the check for a string but I was also thinking we could change how we deal with assemblies with no tests completely in another engine release! |
|
Hm, I pushed this change because this message would have helped me a lot while debug'ing a problem I had. The suggested fix now became a bit bland. |
| { | ||
| var msgNode = loadResult.SelectSingleNode("properties/property[@name='_SKIPREASON']"); | ||
| if (msgNode != null && (new[] { "contains no tests", "Has no TestFixtures" }).Any(msgNode.GetAttribute("value").Contains)) | ||
| TestLog.Info("NUnit couldn't find any tests in " + assemblyName + ". (Probably an assembly dependency problem in your projects."); |
There was a problem hiding this comment.
The (Probably an assembly dependency problem in your projects. bothers me as there are several reasons that this could be the case. I would prefer to leave that part out or at least change it to Possibly. At the very least, the trailing ) is missing in the message.
There was a problem hiding this comment.
That's why I had not approved this earlier, although I can't see my comment here!
This is a perennial problem! From the perspective of the user encountering the problem, it seems that most cases of a particular error probably arise from a certain cause, so they want the message to explain the cause. However, such a message would be misleading in many other cases.
Idea: Can we have a link in the message to an extended explanation? Would a link work in the output window?
There was a problem hiding this comment.
As a developer I understand your concerns, but as a user I would prefer some information instead of none. Some information will help some, no information will help none.
Your idea might work, but what about at tag before such "extra" information, and a disclaimer in the system documentation about what that tag implies? Our just a reference ID, and a documentation page collecting such information? (could even reference back to the commit/PR's where the issue was handled)
Added more information if TestDiscovery went ok, but TestExecuter failed (maybe because of different NUnit versjons in dll's)