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.

Sign up for GitHub

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

Jump to bottom

Fix to #21402 - Test using OData #22367

Merged
merged 1 commit into from
Oct 29, 2020
Merged

Fix to #21402 - Test using OData #22367

merged 1 commit into from
Oct 29, 2020

Conversation

maumar
Copy link
Contributor

@maumar maumar commented Sep 2, 2020

Adding infrastructure and rudimentary tests using OData. Ported Northwind, ComplexNavigations and GearsOfWar models. Only setup for sqlserver currently.

Resolves #21402

smitpatel
smitpatel reviewed Sep 2, 2020
View reviewed changes
EFCore.Runtime.slnf Outdated Show resolved Hide resolved
smitpatel
smitpatel reviewed Sep 2, 2020
View reviewed changes
EFCore.Runtime.slnf Outdated Show resolved Hide resolved
smitpatel
smitpatel reviewed Sep 2, 2020
View reviewed changes
Copy link
Member

@smitpatel smitpatel left a comment

Choose a reason for hiding this comment

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

Are we using OData end to end to generate queries? There is quite a lot of code here which builds/arranges stuff, is this what we are adding to simulate OData or some of it is part of OData query handling?

test/EFCore.OData.Tests/Extensions/HttpContentExtensions.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Extensions/ODataTestExtension.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Extensions/ODataTestExtension.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Extensions/ODataTestExtension.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Query/ComplexNavigationsODataContext.cs Outdated

protected override void OnModelCreating(ModelBuilder modelBuilder)
{
modelBuilder.Entity<Level1>().Property(e => e.Id).ValueGeneratedNever();
Copy link
Member

Choose a reason for hiding this comment

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

Is there any OData specific configuration here?
If not, can we just reuse our context definitions from specs?

Copy link
Contributor Author

@maumar maumar Sep 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I hit some issues with efcore specific stuff, e.g. keyless entities (but that was early on in the development process, could check again). Also the proper context needs to be injected into controllers. Did the easy thing for now just to get tests going. Was planning to tweak this in future checkins

test/EFCore.OData.Tests/Query/ComplexNavigationsODataQueryTests.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Query/ComplexNavigationsODataQueryTests.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Query/GearsOfWarControllers.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Query/GearsOfWarODataQueryTests.cs Outdated Show resolved Hide resolved
test/EFCore.OData.Tests/Query/NorthwindODataQueryTestFixture.cs Outdated Show resolved Hide resolved
@AndriySvyryd
Copy link
Member

You should add an entry to .github/dependabot.yml

AndriySvyryd
AndriySvyryd reviewed Sep 2, 2020
View reviewed changes
test/EFCore.OData.Tests/Query/NorthwindODataContext.cs Outdated
e.Ignore(o => o.ShipPostalCode);
e.Ignore(o => o.ShipRegion);
e.Ignore(o => o.ShipVia);
e.Ignore(o => o.ShippedDate);
Copy link
Member

Choose a reason for hiding this comment

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

Why are so many properties ignored?

Copy link
Member

Choose a reason for hiding this comment

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

That's what we do in northwind, no?

Copy link
Member

Choose a reason for hiding this comment

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

Can we reuse existing code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copied from the original northwind

@AndriySvyryd
Copy link
Member

Do the test initialize their own databases? I.e. if you drop all databases and just run these tests

@maumar
Copy link
Contributor Author

maumar commented Sep 2, 2020

@AndriySvyryd tests fixture inherits from NorthwindQuerySqlServerFixture so it automatically provisions the database when necessary.

@AndriySvyryd
Copy link
Member

@AndriySvyryd tests fixture inherits from NorthwindQuerySqlServerFixture so it automatically provisions the database when necessary.

We shouldn't use the same database across test assemblies as that causes a race condition. Either rename it, use a different model or merge with SQL Server FTs.

@maumar
Copy link
Contributor Author

maumar commented Sep 2, 2020

@AndriySvyryd tests fixture inherits from NorthwindQuerySqlServerFixture so it automatically provisions the database when necessary.

We shouldn't use the same database across test assemblies as that causes a race condition. Either rename it, use a different model or merge with SQL Server FTs.

Will rename. I'm bit scared of adding all the odata dependencies to our existing test suites.

@maumar maumar force-pushed the odata_test_infra_initial branch 2 times, most recently from b23da19 to 82f5eaf Compare September 9, 2020 07:05
@maumar
Copy link
Contributor Author

maumar commented Sep 9, 2020

refactored and updated @smitpatel @AndriySvyryd

@maumar maumar force-pushed the odata_test_infra_initial branch 2 times, most recently from 5e8f44d to 52eb707 Compare September 9, 2020 09:09
bricelam
bricelam approved these changes Sep 10, 2020
View reviewed changes
Copy link
Contributor

@bricelam bricelam left a comment

Choose a reason for hiding this comment

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

LGTM

test/EFCore.OData.FunctionalTests/EFCore.OData.FunctionalTests.csproj Outdated
@@ -0,0 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
Copy link
Contributor

Choose a reason for hiding this comment

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

This concerns me, but then again it might be totally fine.

Copy link
Member

Choose a reason for hiding this comment

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

This is a bit odd. Even in AspNetCore we only use Microsoft.NET.Sdk for tests. Why do you need .Web here? Because of the shared framework dependencies it brings in?

Copy link
Member

Choose a reason for hiding this comment

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

We need HttpGet/ControllerBase/ModelBinder from MVC to run these tests. Reference to Web SDK is needed.

test/EFCore.OData.FunctionalTests/EFCore.OData.FunctionalTests.csproj Outdated
<TargetFrameworks>$(StandardTestTfms)</TargetFrameworks>
<AssemblyName>Microsoft.EntityFrameworkCore.OData.FunctionalTests</AssemblyName>
<RootNamespace>Microsoft.EntityFrameworkCore</RootNamespace>
<IsPackable>true</IsPackable>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to publish this one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it because CI was complaining, will check again without as I changed some stuff there

test/EFCore.OData.FunctionalTests/Query/ComplexNavigationsODataQueryTestFixture.cs Outdated Show resolved Hide resolved
test/EFCore.OData.FunctionalTests/Query/GearsOfWarODataQueryTestFixture.cs Outdated Show resolved Hide resolved
test/EFCore.OData.FunctionalTests/Query/GearsOfWarODataQueryTestFixture.cs Outdated Show resolved Hide resolved
test/EFCore.OData.FunctionalTests/Query/NorthwindODataQueryTestFixture.cs Outdated Show resolved Hide resolved
@maumar
Copy link
Contributor Author

maumar commented Sep 10, 2020

@smitpatel @bricelam do you have a clue why the helix job has failed? the tests seem to be all passing/skipped

@smitpatel
Copy link
Member

Looking at previous iterations of the run and windows is still running, there is some still running process. Runs are getting timed out (even though xunit test run finished).

@maumar maumar force-pushed the odata_test_infra_initial branch 2 times, most recently from 6aae7bf to ca22bab Compare September 12, 2020 21:19
@maumar
Copy link
Contributor Author

maumar commented Sep 18, 2020

@smitpatel ping for the final check

smitpatel
smitpatel reviewed Oct 16, 2020
View reviewed changes
test/EFCore.OData.FunctionalTests/Query/NorthwindODataQueryTestFixture.cs

public IHttpClientFactory ClientFactory { get; private set; }

public override async Task DisposeAsync()
Copy link
Member

Choose a reason for hiding this comment

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

What if we moved _selfHostServer to the interface too and implemented this as DIM? 💡

Tratcher
Tratcher reviewed Oct 20, 2020
View reviewed changes
test/EFCore.OData.FunctionalTests/EFCore.OData.FunctionalTests.csproj Outdated
@@ -0,0 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit odd. Even in AspNetCore we only use Microsoft.NET.Sdk for tests. Why do you need .Web here? Because of the shared framework dependencies it brings in?

test/EFCore.OData.FunctionalTests/Properties/launchSettings.json Outdated Show resolved Hide resolved
test/EFCore.OData.FunctionalTests/Query/EndpointRouteConfiguration.cs Outdated

namespace Microsoft.EntityFrameworkCore.Query
{
public sealed class EndpointRouteConfiguration : IEndpointRouteBuilder
Copy link
Member

Choose a reason for hiding this comment

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

This seems overkill. @javiercn ?

Copy link
Member

Choose a reason for hiding this comment

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

Agree,

I'm not sure what you are trying to achieve here, but there's definitely a better way to do it than this.

test/EFCore.OData.FunctionalTests/Query/ODataQueryTestFixtureInitializer.cs Outdated Show resolved Hide resolved
test/EFCore.OData.FunctionalTests/Query/ODataQueryTestFixtureInitializer.cs Show resolved Hide resolved
test/EFCore.OData.FunctionalTests/Query/ODataQueryTestFixtureInitializer.cs Outdated Show resolved Hide resolved
@maumar
Copy link
Contributor Author

maumar commented Oct 21, 2020

@Tratcher iirc I used blank Asp.net core web app as the basis for the project - I modified the project to Microsoft.NET.Sdk and it works fine

@maumar
Copy link
Contributor Author

maumar commented Oct 27, 2020

@Tratcher @javiercn I have addressed the feedback, please take another look

Tratcher
Tratcher approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

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

The host changes look good. I'm still not sure why you needed to implement a IEndpointRouteBuilder.

javiercn
javiercn reviewed Oct 27, 2020
View reviewed changes
test/EFCore.OData.FunctionalTests/Query/ODataQueryTestFixtureInitializer.cs

namespace Microsoft.EntityFrameworkCore.Query
{
public class ODataQueryTestFixtureInitializer
Copy link
Member

Choose a reason for hiding this comment

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

This is overkill, I would recommend you replace all/most of the infrastructure with the library we ship to do this type of testing. See here for details

Copy link
Member

Choose a reason for hiding this comment

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

I just saw that you are spinning up kestrel, I would avoid that if you can, because it'll make tests faster and much more stable.

@maumar
Fix to #21402 - Test using OData
d9a4350 
Adding infrastructure and rudimentary tests using OData. Ported Northwind, ComplexNavigations and GearsOfWar models. Only setup for sqlserver currently.

Resolves #21402
@maumar maumar merged commit d9a4350 into main Oct 29, 2020
@maumar maumar deleted the odata_test_infra_initial branch October 29, 2020 00:40
@ajcvickers ajcvickers mentioned this pull request Oct 29, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@halter73 halter73 halter73 left review comments

@smitpatel smitpatel smitpatel approved these changes

@AndriySvyryd AndriySvyryd AndriySvyryd left review comments

@javiercn javiercn javiercn left review comments

@bricelam bricelam bricelam approved these changes

@Tratcher Tratcher Tratcher approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Test using OData
7 participants
@maumar @AndriySvyryd @smitpatel @halter73 @bricelam @Tratcher @javiercn