Support reading embedded pdbs #3454
Conversation
|
No tests because I could not find any existing tests for this functionality, but it worked just fine when trying it manually for, embdedded, portable and none types of pdbs. If it throws we swallow the error and don't provide the data, and I am only adding another fallback. So it is low risk. |
| { | ||
| using var stream = new FileHelper().GetStream(pdbFilePath, FileMode.Open, FileAccess.Read); |
There was a problem hiding this comment.
nit: we're using File everywhere in this file maybe it's better use also here or create on top and reuse for future testability.
There was a problem hiding this comment.
This is just the code that was here before. Are you talking about using System.IO.File to create the stream? Or about using FileHelper to improve testability?
There was a problem hiding this comment.
I meant have all FileHelper or all File usage, but it's not mandatory I don't like the mix.
There was a problem hiding this comment.
File helper is used here to get the stream via platform abstractions. It is more correct to use FileHelper everywhere here, but that is a bigger change than what I am comfortable with without adding tests. And I don't want to give this more time right now.
src/Microsoft.TestPlatform.ObjectModel/Navigation/PortableSymbolReader.cs
Outdated
Show resolved
Hide resolved
Description
Building with
<DebugType>embedded</DebugType>embeds portable pdb into the dll. This change either takes the pdb that is next to the file, or falls back to trying to read the pdb information from the executable.Related issue
Fix #1908
Fixes 1. and 2. from #1908 (comment) comment, but not 3. which is a perf improvement). Also keeps the current functionality of looking up the file next to the source in place for backwards compatibility.
If only I saw that comment before I went on to implement this. It spells out exactly what I ended up doing, minus 2 hours spent trying to find the right class and method that can read the embedded pdbs.