Skip to content

feat: add name de-mangling for C++ symbols in the Test Explorer #4340

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

Conversation

rjaegers
Copy link
Contributor

@rjaegers rjaegers commented Mar 8, 2025

This changes visible behavior

The following changes are proposed:

  • Add name de-mangling for C++ symbols in the Test Explorer view

The purpose of this change

Currently when coverage is measured during test execution the mangled name is shown in case of C++ symbols. This PR adds name de-mangling for C++ symbols. When the symbol is not a mangled C++ name, the input is left un-touched.

@rjaegers
Copy link
Contributor Author

rjaegers commented Mar 8, 2025

@gcampbell-msft this is the PR that adds de-mangling. I have added a package to do the actual de-mangling, and I think I should not have pushed the yarn.lock file. I will revert the changes in that file and hope you and your team can do the magic to get the added package onboard.

Copy link
Collaborator

@gcampbell-msft gcampbell-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I don't have any concerns necessarily, but being somewhat unfamiliar with the packages available in node for demangling, I wanted to ask, why did you pick this specific one?

I see that there are a couple of npm packages for it: https://www.npmjs.com/search?q=demangler, and there are a couple that have more downloads (not that more downloads = better quality), and I was curious if there was any reason specifically?

Thanks

@rjaegers
Copy link
Contributor Author

I picked the package that:

  1. had tests
  2. I could review the code of
  3. satisfied my API requirements (i.e. either demangle the mangled name and return that, or return the input untouched)

That left the demangler-js package as most viable option in my opinion.

@rjaegers
Copy link
Contributor Author

@gcampbell-msft is there any further action required from my side to drive this home?

@gcampbell-msft
Copy link
Collaborator

@rjaegers No, nothing from your side, I need to get around to updating the package-lock.json on our side. Thanks for the ping.

@gcampbell-msft
Copy link
Collaborator

@rjaegers Update, I've updated the yarn.lock file and kicked off the builds, we will review and hopefully merge this PR today or next week.

@gcampbell-msft gcampbell-msft enabled auto-merge (squash) April 11, 2025 16:12
gcampbell-msft
gcampbell-msft previously approved these changes Apr 11, 2025
snehara99
snehara99 previously approved these changes Apr 11, 2025
paulmaybee
paulmaybee previously approved these changes Apr 11, 2025
@gcampbell-msft gcampbell-msft dismissed stale reviews from paulmaybee, snehara99, and themself via 1b51bb0 April 16, 2025 12:07
@gcampbell-msft gcampbell-msft added this to the 1.21 milestone Apr 25, 2025
@gcampbell-msft gcampbell-msft merged commit b084cc3 into microsoft:main May 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants