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

Merge benchmarks back into main repo #7757

Merged
merged 1 commit into from Mar 2, 2017
Merged

Merge benchmarks back into main repo #7757

merged 1 commit into from Mar 2, 2017

Conversation

rowanmiller
Copy link
Contributor

Resolves #7625

In a non-perf run (i.e. when you just run locally, and on CI) the tests take 68sec to execute on my laptop - so probably under a minute on most hardware.

@bricelam could you take a quick look over this one. It is mostly just a copy of the code except I changed assembly/project names (and namespaces) around.

  • Benchmarks.Core (shared infra) => Microsoft.EntityFrameworkCore.Benchmarks
  • Benchmarks (EF Core benchmarks) => Microsoft.EntityFrameworkCore.Benchmarks.EFCore
  • Benchmarks.EF6 (EF6 benchmarks) => Microsoft.EntityFrameworkCore.Benchmarks.EF6

I'll also add a comment to the other piece of code I changed.

return ".NET Framework";
#else
return ".NET Core";
#endif
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used to use Microsoft.Extensions.Internal.RuntimeEnvironment.RuntimeType but that now returns CoreCLR even when running on the net452 output in the xunit console runner. What I have now is what we used to have before we split the benchmarks out to a separate repo (where the compilation of tests may not have matched the compilation of product).

}

[Benchmark]
[AdventureWorksDatabaseRequired]
Copy link
Member

Choose a reason for hiding this comment

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

Do we need attribute on each method if the class is already marked with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly not, I'll play around with that after I've got the code merged. I wanted to move it over with minimal changes to start with 😄

Copy link
Member

@ajcvickers ajcvickers left a comment

Choose a reason for hiding this comment

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

I'm happy with getting this merged in first and then making additional changes as necessary.

@ErikEJ
Copy link
Contributor

ErikEJ commented Mar 1, 2017

This is great, looking forward to try them out with the SQLCE provider

<Project Sdk="Microsoft.NET.Sdk">
<Import Project="..\..\tools\EntityFramework.props" />
<PropertyGroup>
<TargetFrameworks>net452</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be able to get away with just <TargetFramework> (no s) here if the EF and ASP.NET targets can handle that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Might also need to exclude this from non-Windows. @natemcmaster probably knows how to do that...

Copy link
Contributor

Choose a reason for hiding this comment

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

@bricelam
Copy link
Contributor

bricelam commented Mar 1, 2017

LGTM (just looked at *.csproj files)

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.

None yet

7 participants