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

Implement `--list` - fixes #905 #914

Merged
merged 5 commits into from Oct 16, 2018

Conversation

Projects
None yet
4 participants
@wojtpl2
Copy link
Collaborator

commented Oct 16, 2018

Fix for #905

I've implemented '--list:[flat|tree]'.

Now you should be able to run BenchmarkDotNet.Samples.exe with options:

BenchmarkDotNet.Samples.exe --list flat

BenchmarkDotNet.Samples.Algo_Md5VsSha256.Md5
BenchmarkDotNet.Samples.Algo_Md5VsSha256.Sha256
BenchmarkDotNet.Samples.IntroArguments.Benchmark
BenchmarkDotNet.Samples.IntroArgumentsSource.SingleArgument
BenchmarkDotNet.Samples.IntroArgumentsSource.ManyArguments
BenchmarkDotNet.Samples.IntroArrayParam.ArrayIndexOf
BenchmarkDotNet.Samples.IntroArrayParam.ManualIndexOf
BenchmarkDotNet.Samples.IntroBasic.Sleep
BenchmarkDotNet.Samples.IntroBasic.Thread.Sleep(10)
[...]

BenchmarkDotNet.Samples.exe --list tree

BenchmarkDotNet
 └─Samples
    ├─Algo_Md5VsSha256
    │  ├─Md5
    │  └─Sha256
    ├─IntroArguments
    │  └─Benchmark
    ├─IntroArgumentsSource
    │  ├─SingleArgument
    │  └─ManyArguments
    ├─IntroArrayParam
    │  ├─ArrayIndexOf
    │  └─ManualIndexOf
    ├─IntroBasic
    │  ├─Sleep
    │  └─Thread
    │     └─Sleep(10)
[...]

You can also use filter option, like this:

BenchmarkDotNet.Samples.exe --list flat --filter *IntroSetupCleanup*

BenchmarkDotNet.Samples.IntroSetupCleanupGlobal.Logic
BenchmarkDotNet.Samples.IntroSetupCleanupIteration.Benchmark
BenchmarkDotNet.Samples.IntroSetupCleanupTarget.BenchmarkA
BenchmarkDotNet.Samples.IntroSetupCleanupTarget.BenchmarkB
BenchmarkDotNet.Samples.IntroSetupCleanupTarget.BenchmarkC
BenchmarkDotNet.Samples.IntroSetupCleanupTarget.BenchmarkD

BenchmarkDotNet.Samples.exe --list tree --filter *IntroSetupCleanup*

BenchmarkDotNet
 └─Samples
    ├─IntroSetupCleanupGlobal
    │  └─Logic
    ├─IntroSetupCleanupIteration
    │  └─Benchmark
    └─IntroSetupCleanupTarget
       ├─BenchmarkA
       ├─BenchmarkB
       ├─BenchmarkC
       └─BenchmarkD
@dnfclas

This comment has been minimized.

Copy link

commented Oct 16, 2018

CLA assistant check
All CLA requirements met.

@adamsitnik
Copy link
Member

left a comment

@wojtpl2 Great implementation of a really good idea! I can't wait to merge this PR!

I added some comments, mostly very minor things. Could you please fix them?

using System;

namespace BenchmarkDotNet.ListBenchmarks {

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

nit: { should be in a separate line


PrintNode(node, indent);
}

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

nit: extra empty line


void PrintChildNode(Node node, string indent, bool isLast)
{
// Print the provided pipes/spaces indent

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

nit: this comment is not necessary. When I see Console.Write(ident) I know that we are printing indent because the method argument has a descriptive name

private readonly IBenchmarkCasesPrinter printer;

public BenchmarkCasesPrinter(ListBenchmarkCaseMode listBenchmarkCaseMode)
{

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

this method could be simplified to

=> listBenchmarkCaseMode ==  ListBenchmarkCaseMode.Tree
        ? new TreeBenchmarkCasesPrinter()
        :  new FlatBenchmarkCasesPrinter();

or if we want to keep the switch we should have 3 cases: Tree, Flat and Disabled (throw)

{
internal interface IBenchmarkCasesPrinter
{
void Print(IEnumerable<string> testName);

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

it should be testNames (missing s)

}

public void Print(IEnumerable<string> testName)
{

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

nit: good place to use an expression body Print(IEnumerable<string> testNames) => printer.Print(testNames);

using System.Collections.Generic;
using System.Linq;

namespace BenchmarkDotNet.ListBenchmarks {

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

nit: { should be in a separate line

PrepareNodeTree(topLevelNodes, partsOfName);
}

var tree = new AsciiTreeDiagram();

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

personally, I would merge the body of AsciiTreeDiagram into this class. Both have the same responsibility - printing a tree. We could keep AsciiTreeDiagram if it had some unit tests

if (listBenchmarkCase)
{
var testName = filteredBenchmarks.SelectMany(p => p.BenchmarksCases)
.Select(p => p.Descriptor.Type.Namespace + "." + p.Descriptor.DisplayInfo).Distinct();

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

the benchmarks are filtered using following code:

string fullBenchmarkName = benchmarkCase.Descriptor.GetFilterName();

could you please use benchmarkCase.Descriptor.GetFilterName() instead of p.Descriptor.Type.Namespace + "." + p.Descriptor.DisplayInfo ?

@@ -0,0 +1,51 @@
using System;

namespace BenchmarkDotNet.ListBenchmarks {

This comment has been minimized.

Copy link
@adamsitnik

adamsitnik Oct 16, 2018

Member

could you please move all of the types from BenchmarkDotNet.ListBenchmarks to BenchmarkDotNet.ConsoleArguments.ListBenchmarks? BDN project has already too many folders, we try to avoid adding new ones on the root level

@wojtpl2

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 16, 2018

Yes, great suggestions. I'll fix it.

wojtpl2 added some commits Oct 16, 2018

@wojtpl2

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 16, 2018

Now everything is fixed.

@adamsitnik adamsitnik merged commit 330f66c into dotnet:master Oct 16, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@adamsitnik

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@wojtpl2 thank you! awesome PR!

@AndreyAkinshin AndreyAkinshin added this to the v0.11.2 milestone Oct 16, 2018

Ky7m added a commit to Ky7m/BenchmarkDotNet that referenced this pull request Oct 16, 2018

Merge branch 'master' into azure-pipelines
* master:
  Implement `--list` - fixes dotnet#905 (dotnet#914)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.