Skip to content

Fix duplicated test name in Perf.TypeName and update validation #4898

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

caaavik-msft
Copy link
Contributor

@caaavik-msft caaavik-msft commented Aug 5, 2025

Due to how the FullNameProvider works, the Dictionary<string, bool>[] and Dictionary<List<int[]>[,], List<int?[][][,]>>[] arguments were both being converted to the string System.Collections.Generic.Dictionary`2[]. Patching this in BenchmarkDotNet would cause too many test names to change, and this is the only case in this repo that there is a duplicate full name, so we can address it by wrapping the type and overriding the ToString.

The display names that I have put here ensure that the test names remain the same as they were before, with only the Dictionary<List<int[]>[,], List<int?[][][,]>>[] case now being converted to typeof(System.Collections.Generic.Dictionary`2[]) (COMPLEX) to differentiate it from the simpler case.

I have also updated the validators to ensure this doesn't happen again.

@caaavik-msft
Copy link
Contributor Author

@adamsitnik As you had originally added this benchmark, are you able to confirm that the changes to the benchmark by using a wrapper type are acceptable and that adding the property lookup should have a negligible impact on the performance measurement?

Copy link
Member

@LoopedBard3 LoopedBard3 left a comment

Choose a reason for hiding this comment

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

Looks good.

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.

2 participants