Skip to content

Conversation

@Shazwazza
Copy link
Contributor

referenced issue: #290

Some notes on this PR:

  • Original PR was here Supporting testing between Nuget dependencies #805 but the code became out-dated with recent changes to the benchmark.net project. There are some interesting notes about running benchmarks between Nuget versions Supporting testing between Nuget dependencies #805 (comment) (will summarize more below)
  • This only supports running with the CsProjCoreToolchain, I've added a couple TODO notes in the Roslyn/Generator.cs since I think it might be possible to use the roslyn toolchain for this but I think will require a bunch of manual nuget.exe work (?)
  • Have added an IntroNuget sample. As per the notes mentioned above, this uses Reflection in the sample so that we don't need a direct reference to the Newtonsoft.Json, however if you think it would be more clear as far as sample code goes, I can add a direct reference to the minimum version tested of Newtsonsoft.Json to demonstrate how a benchmark project should be setup to test between different Nuget versions... just let me know.

How to benchmark between Nuget versions

Ideally this is how it is done:

  • The benchmark project references the minimum version of the nuget dependency that is being tested in the list of Jobs so that reflection doesn't have to be used to call the APIs and it builds
  • The list of Jobs specifies the exact version that will be tested and that is what will be resolved from Nuget
  • So long as the API signatures haven't changed between all of these versions then it should 'just work'

When each benchmark is run, it will add the correct nuget package dependency and build the project with that specific version and run the benchmarks against it.

This scenario will not work if API signatures have changed between versions. In those cases, different benchmark projects would need to be setup for the same API surface area and then the resulting reports could be combined.

# Conflicts:
#	src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliBuilder.cs
#	src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliCommandExecutor.cs
#	src/BenchmarkDotNet/Toolchains/DotNetCli/DotNetCliGenerator.cs
#	src/BenchmarkDotNet/Toolchains/Roslyn/Generator.cs
Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

@Shazwazza great PR! Honestly I was afraid that this feature will require a lot of code changes but you made great reuse of existing components and wrote it in really simple and smart way! 👍

Before we merge it could you please:

  1. Simplify the IntroNuget and add a simple doc page to our docs? You can take a look into docs\articles\samples for examples.
  2. Add at least one test? Could be sth like:
public class NugetReferenceTests : BenchmarkTestExecutor
{
    public NugetReferenceTests(ITestOutputHelper output) : base(output) { }

    [Fact]
    public void UserCanSpecifyCustomNuGetPackageDependency()
    {
        var toolchain = RuntimeInformation.IsFullFramework
            ? CsProjClassicNetToolchain.Current.Value // this .NET toolchain will do the right thing, the default RoslynToolchain does not support it
            : CsProjCoreToolchain.Current.Value;

        var job = Job.Dry.With(toolchain).WithNuget("Newtonsoft.Json", "11.0.2");
        var config = CreateSimpleConfig(job: job);

        CanExecute<WithCallToNewtonsoft>(config);
    }

    public class WithCallToNewtonsoft
    {
        [Benchmark] public void CallNewton() { } // the code that you added to IntroNuget would be great
    }
}

@Shazwazza
Copy link
Contributor Author

@adamsitnik I've pushed a few more commits to address your review including a few tests, let me know if there's anything i've missed

@adamsitnik adamsitnik merged commit 92a7869 into dotnet:master Oct 25, 2018
@adamsitnik
Copy link
Member

@Shazwazza thank you! great job!

@AndreyAkinshin AndreyAkinshin added this to the v0.11.2 milestone Oct 25, 2018
@Shazwazza
Copy link
Contributor Author

Whoohoo! awesome :) 🎉

@Shazwazza
Copy link
Contributor Author

Hi @adamsitnik , I was wondering if there's already a way in BDN to specify nuget package sources? Reason I'm asking is because perhaps the https://github.com/dotnet/BenchmarkDotNet/blob/master/src/BenchmarkDotNet/Jobs/NugetReference.cs can be extended to have a 'Source' property which is then used when calling the add package cmd? Would be happy to make that change if it makes sense just let me know. Cheers!

@adamsitnik
Copy link
Member

I was wondering if there's already a way in BDN to specify nuget package sources?

the only way is to use a nuget.config file (as for every msbuild project)

if this is not enough for your scenario, a PR would be very welcomed as usual

thanks!

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.

3 participants