Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support embedded PDBs in OM #1908

Closed
shyamnamboodiripad opened this issue Jan 31, 2019 · 6 comments · Fixed by #3454
Closed

Support embedded PDBs in OM #1908

shyamnamboodiripad opened this issue Jan 31, 2019 · 6 comments · Fixed by #3454

Comments

@shyamnamboodiripad
Copy link
Contributor

Description

OM currently supports regular and portable PDBs (see https://github.com/Microsoft/vstest/tree/master/src/Microsoft.TestPlatform.ObjectModel/Navigation). However, as far as I can tell, it doesn't support embedded pdbs.

Not sure if this is the same issue as #1748 but is probably related.

Steps to reproduce

VS's Test Window uses ISourceInformationProvider to side step PDB based source information lookup in most cases for managed projects. However, ISourceInformationProvider currently does not support multi-target-framework projects - so we end up falling back to PDB based source information for such projects. Now if such project also happens to use embedded PDB then the source information lookup fails and we end up not finding tests corresponding to a particular file.

To reproduce create a test project that is both mtfm as well as uses embedded PDB and try to run tests by right clicking on the test file in the solution explorer in any 16.0 VS build.

Expected behavior

Tests in the file should be executed (and this works if I switch the repro project to use portable PDB instead of embedded).

Actual behavior

No tests are found for selected file.

@shyamnamboodiripad
Copy link
Contributor Author

cc @peterwald

@shyamnamboodiripad
Copy link
Contributor Author

@pvlakshm Embedded pdb support was mentioned in one of your updates on the internal alias last summer - but I believe that update was mainly around code coverage tooling. Is the OM support something you are working on currently?

@shyamnamboodiripad
Copy link
Contributor Author

shyamnamboodiripad commented Oct 30, 2019

@tmat Could you please comment on what would be needed to fix the TP code linked above to support embedded PDBs?

We are running into more cases where it is not possible to run tests from VS editor / navigate to tests from VS test explorer because adapter-discovered tests (for NUnit, xUnit and MSTest) fail to retrieve source information when embedded PDBs are turned on. Adapters are currently using the above helper to retrieve source information. Things work fine for languages like VB and C# that support Roslyn-based test discovery and source navigation (since we are able to retrieve the source information via Roslyn without having to look at PDBs). However they don't work for F#.

@shyamnamboodiripad
Copy link
Contributor Author

Also tagging @peterwald @AbhitejJohn

@shyamnamboodiripad
Copy link
Contributor Author

Synced up with @tmat offline and here's what he recommended we would need to do to support embedded pdbs -

  1. This code is incorrect. Portable pdbs need not necessarily be present next to the binary being built. We need to call PEReader.TryOpenAssociatedPortablePdb() instead. This handles both embedded and non-embedded portable pdbs.
  2. The above method returns MetadataReaderProvider from we can retrieve a MetadataReader for the pdb.
  3. We can then replace this code which is reflection loading the assembly and traversing all the contained types and methods with code to do the same visitation using System.Reflection.Metadata.MetadataReader. And we can look up source information for the visited methods in the pdb MetadataReader in 2.

Avoiding reflection assembly load and type resolution in 3 above would also speed up the source information lookup (which we need to perform on every build for non-C# and non-VB projects) and also avoid concerns around loading the test assemblies into the test host process...

cc @ManishJayaswal @peterwald @AbhitejJohn

(Note: The original mtfm issue mentioned in the repro steps is no longer relevant as we don't use ISourceInformationProvider (SIP) in the test explorer anymore)

@nohwnd
Copy link
Member

nohwnd commented Mar 9, 2022

Moved 3. to #3455

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants