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

Create tests for GetNavigationSource extension method #1159

Merged
merged 12 commits into from
Jan 23, 2024

Conversation

habbes
Copy link
Contributor

@habbes habbes commented Jan 19, 2024

Related to issue #1118

Related to PR #1158

I want to refactor the ODataPathExtensions.GetNavigationSource extension method to improve its performance, but that method doesn't have tests. This PR adds tests covering all supported scenarios. This will make it safer to refactor without breaking existing behaviour.

[InlineData("VipCustomer/NS.SpecialCustomer", "VipCustomer", EdmNavigationSourceKind.Singleton)]
[InlineData("Customers/$count", null, EdmNavigationSourceKind.None)]
[InlineData("Customers(1)/Account/Amount/$value", null, EdmNavigationSourceKind.None)]
[InlineData("$batch", null, EdmNavigationSourceKind.None)]

Choose a reason for hiding this comment

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

Is there a way to label these tests so as as to show which test case is being covered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We mostly use comments for such cases. Is that fine?

Choose a reason for hiding this comment

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

I suppose that works just as well.

Copy link
Contributor

@gathogojr gathogojr Jan 19, 2024

Choose a reason for hiding this comment

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

@wachugamaina @habbes FYI xunit/xunit#2532

@wachugamaina Curious, do you use NUnit in your repos? Does NUnit support parameterized tests with labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gathogojr thanks for the tip. That DisplayName features in that thread will be part of xunit v3 which is not out yet.

wachugamaina
wachugamaina previously approved these changes Jan 19, 2024
wachugamaina
wachugamaina previously approved these changes Jan 19, 2024
@@ -290,6 +290,31 @@ public CustomersModelWithInheritance()
getOrder.AddParameter("orderId", intType);
model.AddElement(getOrder);

// functions with entity set
Copy link
Contributor

@ElizabethOkerio ElizabethOkerio Jan 22, 2024

Choose a reason for hiding this comment

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

what does this mean? bound to entity set? functions with entity set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Functions that have an entity set path set. I think the entity set path expression is linked to the collection that contains the return type for functions that return an entity.

"GetTopCustomers",
new EdmCollectionTypeReference(new EdmCollectionType(new EdmEntityTypeReference(customer, false))),
isBound: false,
entitySetPathExpression: customers.Path,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you have a path expression for an unbound function?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I believe it applies for functions that return an entity or entity collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the comment to reflect that this refers to the entity set path.

Comment on lines +90 to +91
// unbound operation import with an entity set path should return the entity set
[InlineData("GetTopCustomers()", "Customers", EdmNavigationSourceKind.EntitySet)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question @ElizabethOkerio asked. How does an unbound operation import have a navigation source?

Copy link
Contributor Author

@habbes habbes Jan 22, 2024

Choose a reason for hiding this comment

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

This means that the function's returned values are entities from the specified entity set.

Copy link
Contributor

Choose a reason for hiding this comment

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

@habbes How is such a function configured?

Copy link
Contributor

@gathogojr gathogojr Jan 22, 2024

Choose a reason for hiding this comment

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

@habbes I guess:

modelBuilder.Function("GetTopCustomers").ReturnsCollectionFromEntitySet<Customer>("Customers");

Copy link
Contributor

Choose a reason for hiding this comment

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

What I didn't know is that it returns the specified entity set as the navigation source...

gathogojr
gathogojr previously approved these changes Jan 22, 2024
Copy link
Contributor

@gathogojr gathogojr left a comment

Choose a reason for hiding this comment

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

Left a few comments; otherwise LGTM

Comment on lines +84 to +85
// property segment with a complex type, returns navigation source of previous segment
[InlineData("Customers(1)/Account", "Customers", EdmNavigationSourceKind.EntitySet)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a property segment here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the property segment has a complex type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/Account is the property segment. Account is a complex property. And so the navigation source here is the navigation source of the previous segment, which is the key segment (1), whose navigation source is the Customers entity set.

Co-authored-by: John Gathogo <john.gathogo@gmail.com>
@habbes habbes merged commit 1b37400 into OData:main Jan 23, 2024
2 checks passed
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.

4 participants