ISourceInformationProvider doesn't work for async methods
bradwilson opened this issue · 14 comments
It appears that, when given async methods, ISourceInformationProvider does not return source information.
This is our usage: https://github.com/xunit/dnx.xunit/blob/master/src/xunit.runner.dnx/DesignTime/SourceInformationProvider.cs
Related issue: xunit/xunit#530
It's probably worth noting that our implementation against the one built into Visual Studio needs to scout around for the state machine attribute, and ask for the MoveNext method instead. Should this code also be necessary for DNX's ISourceInformationProvider, or is that logic already built into your implementation?
We'd prefer to fix the defects in our implementation rather than require workarounds. I've done some prototyping using IL metadata reader, and it might get us closer to being able to fix these issues.
@bradwilson - if we created another overload that accepted a metadata token would you be able to use that ? https://msdn.microsoft.com/en-us/library/system.reflection.memberinfo.metadatatoken(v=vs.110).aspx
The defects I'm aware of (all cases where the class/method name don't match the actual user code):
- Inherited methods
- Overloaded methods
- Compiler-generated methods (async, iterators)
In any of these cases, the method in question will show up as "unknown" in the VS Test Explorer when grouped by project, and source-code related features of VS won't work.
Well, I have a MethodInfo, so assuming that DNX allows me access to MetadataToken, it shouldn't be a problem.
Nope, MetadataToken does not appear to be an option:
Sad panda 🐼
Would it be feasible in xUnit to give us the MethodInfo? I'm not sure the historical reason why VS did it with the class name + method name, but giving us the MethodInfo directly would eliminate some ambiguity.
Yeah, that shouldn't be a problem. During discovery for DNX I always have the MethodInfo handy.
For now I'm adding support for async/inherited methods based on the class/method names.
We should talk about how to stage the transition of this API from names -> MethodInfo in a follow up. It won't be trivial to migrate our bad internal fork of xUnit. We could do something like add a MethodInfo overload to this API and then remove the string-based one when we're ready to kill off our fork.
We should talk about how to stage the transition of this API from names -> MethodInfo in a follow up. It won't be trivial to migrate our bad internal fork of xUnit. We could do something like add a MethodInfo overload to this API and then remove the string-based one when we're ready to kill off our fork.
Actually, I take that back. We'll just do the reflection lookup in our fork and the remove the string-based-api. That makes this easy.
Just let me know when this makes it to /F/aspnetvnext (which is currently broken 😞).
As for your private runner build, the change should be fairly trivial. If you look at this code:
The testCase.TestMethod.Method used here is typed as IMethodInfo, because we allow third parties to discover tests and they may do so without binaries (like Resharper and CodeRush). However, you should be able to test to see if it's actually IReflectionMethodInfo, which it almost always will be (since in this case, we know discovery is almost always done by us using reflection), and if it is, use the MethodInfo property on it.
I'm planning to let a build push to to the feed with the new functionality (but the same API), and then make the changes to the API in a separate change. A few things were red today when I came in, and we haven't had a coherence push since .
FYI also about https://github.com/dotnet/corefx/issues/2375#issuecomment-129711086
We may get MetadataToken for CoreCLR after all. Plan to go ahead with the changes to use MethodInfo anyway because that will be better for dealing with async. MetadataToken will allows us to fix the cases for overloaded methods when it's available.
This has been checked in. Still waiting for a public build to be available.
@bradwilson - this is available now via the aspnetvnext feed. v1.0.0-beta7-11855 https://www.myget.org/gallery/aspnetvnext