-
-
Notifications
You must be signed in to change notification settings - Fork 984
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
Show toolchain column when multiple toolchains are used #2121
base: master
Are you sure you want to change the base?
Conversation
5375828
to
6b698f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am afraid this is trickier that it looks like.
Currently when I run
cd samples\BenchmarkDotNet.Samples
dotnet run -c Release -f net6.0 --filter *IntroBasic.Sleep --join --runtimes net462 net6.0 --job dry
I get:
With your changes the toolchain column becomes visible again:
We want the toolchain column to be visible only when there are at least two benchmarks that use the same Runtime, but different Toolchain.
ad42f4c
to
583bb66
Compare
It looks that we should never display the
I think we should disallow setting |
1380f38
to
27aec58
Compare
|
||
[Theory] | ||
[InlineData("nativeaot6.0", "NativeAOT 6.0", "ILCompiler 6.0.0-*")] | ||
[InlineData("nativeaot7.0", "NativeAOT 7.0", "Latest ILCompiler")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Latest ILCompiler
the desired toolchain name? Should it be ILCompiler 7.0.0-*
?
Ready for review. The only problem I found is: BenchmarkDotNet/tests/BenchmarkDotNet.Tests/Columns/JobColumnsApprovalTests.cs Lines 92 to 97 in aa8ef43
I didn't dare to change a default toolchain. |
aa8ef43
to
371ee5d
Compare
Done
|
371ee5d
to
2c99d26
Compare
Some possible improvements:1) The
|
The 3rd commit shows what's changed.
I removed
Summary.IsMultipleRuntimes
because it is actually redundant.Now the
Toolchain
column will be showed only when using.WithToolchain()
method.I tried shortening the approval filenames, but they are still long