Support base-class tests from external assemblies #315
Conversation
CharliePoole
changed the title
|
The PR looks like an improvement. I'm vaguely aware that we recently made almost the same change, however, so I want to look more closely when I'm back home with my development system in a few hours. This doesn't explain that you are seeing the same problem with the console runner, since your fix would not have anything to do with that. |
There was a problem hiding this comment.
This resolves the case although it doesn't go as far as @OsirisTerje 's fix in PR nunit/nunit-vs-adapter#149. That fix handles types that are defined in a referenced assembly located in a different directory. @OsirisTerje do you think that's necessary in the VS environment?
|
I approved but with a question for @OsirisTerje. We need his second review to merge. Thanks for your work! |
|
@CharliePoole Sorry that I didn't find the existing work. It didn't occur to me that searching for "navigation" would turn up some work in progress, though that's rather obvious in hindsight... Thanks for integrating it ;-) |
|
Well it's in a different repo and so hard to find unless you know it's there. |
* Remove inactive code * When searching for navigation data, make the iteration through the base classes explicit
|
I've now integrated the changes from the nunit2 adapter, and cleaned up the code a bit for readability. Since this navigation problem is strictly dependent on having multiple assemblies, should we extend the NavigationDataTests (that is, introduce another test assembly), or do you use separate integration tests for such issues? |
|
@gimpf Good that you integrated those changes, and yes, @CharliePoole , I think that is necessary, this was what solved the 2.X adapter issue. I am not sure exactly how this works out with the different Mono.Cecil libs, but I would better be safe than sorry here. |
When resolving base-types via `TypeReference.Resolve()`, no symbols are read. As a result, base-classes in external assemblies could not provide navigation-info (SequencePoint would always be null). Also, while reworking the operations around the `.Resolve()`, also catch any assembly resolution error, as we do not have any guarantee that our Mono.Cecil search path really is identical to the .NET's one.
|
@OsirisTerje I've now added two additional tests for the NavigationDataProvider for types that reference other assemblies. Also, since the first time I opened the PR, I've refactored some things, you might want to revisit the code. Those two new tests brought to light another issue regarding the navigation-data: The way we resolved the base-types caused that no navigation-data for the types was loaded. I remember that you mentioned an issue regarding "cannot jump to source" in the linked issue. That might be resolved now as well (though I haven't checked). Regarding the PR itself, I from my side consider it complete, but I'll remain available for fixing issues from code reviews or tests. Thanks for your support! |
|
@OsirisTerje I was just trying to figure out whether the use case (base assembly in a different directory) can arise in normal use. I guess it would require the user to define two different output directories and turn off local copy... odd but possible. @gimpf Re-reviewing! |
|
This still looks good to me. |
|
This looks good to me too. @OsirisTerje shall we merge? I will need to merge this into my PR. |
|
@rprouse Yep, looks fine to me too. Your call :-) |
|
Is this change released? |
|
@kantnisha yes, this is in the current released version. |
|
thanks |
|
Does this work on VS2017? Anyone tried? |
|
See comments on #314 and also try the repro here on the NUnit3 branch, https://github.com/OsirisTerje/NUnitTestAdapterIssue143 |
Fixes #314
The problem was caused because the Mono.Cecil failed to resolve types from external assemblies, even if they are in the same directory.
This is fixed by explicitly adding the directory of the base-library to the search-path for Mono's assembly-resolver.
This fix works for me, but I'd have to lie when I'd say I've tested it extensively. Please see gimpf/nunit3-vs-adapter-issue314 for a test-case. It fails using the regular adapter, but works using the fixes from this PR.