Skip to content

Revert "Add [xunit*]* to default excluded modules filter if not specified" #519

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

Merged
merged 1 commit into from
Aug 28, 2019

Conversation

MarcoRossignoli
Copy link
Collaborator

@MarcoRossignoli MarcoRossignoli commented Aug 14, 2019

Reverts #462

With #510 now we exclude from instrumentation module with embedded pdb without local source.
For this reason we could think to remove xunit filter from code as @tonerdo told here #510 (comment)
We could keep filter as fallback in case that xunit for some reason will move embedded pdb to local folder(@onovotny talked about it here #510 (comment))

Personally I would keep filter for the moment, but yes could be unuseful/dead code.

Thoughts? @tonerdo @vagisha-nidhi @onovotny @ViktorHofer
Maybe @bradwilson could also be useful here to confirm or not something.

Ah one more thing, if someone know the name of nuget package that download pdb on output directory please let me know...so we can write code/test to skip also that case.

@MarcoRossignoli MarcoRossignoli added the enhancement General enhancement request label Aug 14, 2019
@tonerdo
Copy link
Collaborator

tonerdo commented Aug 28, 2019

@MarcoRossignoli what do you mean by

We could keep filter as fallback in case that xunit for some reason will move embedded pdb to local

This change seems legit to me as it's unlikely for the xunit source files to be on a user's machine except they work on xunit itself

@MarcoRossignoli
Copy link
Collaborator Author

MarcoRossignoli commented Aug 28, 2019

This change seems legit to me as it's unlikely for the xunit source files to be on a user's machine except they work on xunit itself

@tonerdo I mean that for the moment we check for source only for embedded pdb, so if for some reason(unlikely I think) move pdb to local target folder we won't check for source but we'll try to instrument

@tonerdo
Copy link
Collaborator

tonerdo commented Aug 28, 2019

Oh I see. Well we should update the code to check for local sources for both embedded and standalone pdb

@MarcoRossignoli
Copy link
Collaborator Author

Oh I see. Well we should update the code to check for local sources for both embedded and standalone pdb

I agree as I've asked to Oren I would like to write a test on it, but I need a "sample" nuget package that behave like that (before build one tailored made)...@tonerdo do you know one?

@clairernovotny
Copy link
Contributor

Try Microsoft.Toolkit. It has a pdb in it with sourcelink data in it:

image

@MarcoRossignoli
Copy link
Collaborator Author

Great thanks Oren! I'm on vacancy until next week, I'll give it a try asap!

@bradwilson
Copy link

I can't imagine we (@xunit) would ever go back to external PDBs.

@MarcoRossignoli
Copy link
Collaborator Author

MarcoRossignoli commented Aug 28, 2019

Thanks @bradwilson for info
@tonerdo we can merge for now...I'll update with external pdbs on new PR

@MarcoRossignoli MarcoRossignoli merged commit 15eeb98 into master Aug 28, 2019
@MarcoRossignoli MarcoRossignoli deleted the revert-462-defaultexcludedfiles branch August 28, 2019 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General enhancement request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants