Skip to content
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

System.Runtime #112

Merged
merged 31 commits into from
Aug 15, 2018
Merged

System.Runtime #112

merged 31 commits into from
Aug 15, 2018

Conversation

adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Aug 10, 2018

Fixes #68

@ViktorHofer after I am done with this task I will be taking other tasks that are not finished, even if they are assigned to someone else ;p

For the first time of CoreFX port I have hit a bug in BDN: dotnet/BenchmarkDotNet#851 and dotnet/BenchmarkDotNet#850

…umentsSource to show devs how MANY test cases we have, -21 benchmarks
@adamsitnik
Copy link
Member Author

image

@jorive I added a simple validator to prevent the people from adding too many test cases with ArgumentsSource/MemberData

I did that after I found few benchmarks with HOUNRDRES of test cases...

@@ -0,0 +1,29 @@
using System.Collections.Generic;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@adamsitnik adamsitnik changed the title [WIP] System.Runtime System.Runtime Aug 14, 2018
@adamsitnik
Copy link
Member Author

Ready for review, I have ported the benchmarks and removed a LOT of test cases that very often were providing no value. Before we had 1500 benchmarks in "System.Tests" namespace, now 212.

=> string.Format(s, o);

[Benchmark]
public string Format_MultipleArgs()
Copy link
Member

@jorive jorive Aug 14, 2018

Choose a reason for hiding this comment

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

Do we need a similar case for string interpolation? For example:

$"{arg1} {arg2}"
``` #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

good idea! I added the test case for string interpolation


[Benchmark]
[Arguments("This is a very nice sentence", 'z', 'y')] // 'z' does not exist in the string
[Arguments("This is a very nice sentence", 'i', 'I')] // 'i' occuress 3 times in the string
Copy link
Member

@jorive jorive Aug 15, 2018

Choose a reason for hiding this comment

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

nit: "This is a very nice benchmark sentence" ;) … hehe #Closed

Copy link
Member

@jorive jorive Aug 15, 2018

Choose a reason for hiding this comment

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

do not need to change it! #Closed

Copy link
Member

@jorive jorive left a comment

Choose a reason for hiding this comment

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

:shipit:

@adamsitnik adamsitnik merged commit 7ec7725 into dotnet:master Aug 15, 2018
@adamsitnik adamsitnik deleted the runtime branch October 17, 2018 15:04
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