-
Notifications
You must be signed in to change notification settings - Fork 473
Cleanup ODataIdResolvers and E2E tests #2693
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
Cleanup ODataIdResolvers and E2E tests #2693
Conversation
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
test/E2ETest/Microsoft.Test.E2E.AspNet.OData/BulkOperation/BulkInsertController.cs
Show resolved
Hide resolved
test/E2ETest/Microsoft.Test.E2E.AspNet.OData/BulkOperation/BulkInsertController.cs
Show resolved
Hide resolved
test/E2ETest/Microsoft.Test.E2E.AspNet.OData/BulkOperation/BulkInsertController.cs
Outdated
Show resolved
Hide resolved
test/E2ETest/Microsoft.Test.E2E.AspNet.OData/BulkOperation/BulkInsertController.cs
Show resolved
Hide resolved
| } | ||
|
|
||
| [Fact] | ||
| public async Task PostEmployee_WithCreateFriends() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to add more tests here - with containment, typesegments
habbes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite understand the changes in this PR or what problem it solves. Could you add a description and more context to the PR, and link related issues if any?
| /// <summary> | ||
| /// Extensions method for <see cref="ODataPath"/>. | ||
| /// </summary> | ||
| public static class ODataPathExtensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this changed from internal to public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For use by the customers using the nuget package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, Was there actual demand for these path extensions, or this is done in anticipation? (I know the ones in the core library were explicitly asked for)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in anticipation.
Also we are using them in E2E tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KenitoInc E2E tests would hardly be a reason to make a method public. We use the friend library feature for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, we should keep them internal unless we have concrete use cases in mind where we know customers will need them for, or customers have explicitly asked for them. Making things public increases the maintenance overhead in my opinion. It makes internal improvements and refactoring difficult and changes to their signature almost impossible. But if you still think they should be public, I'm not opposed to that. I but I would suggest considering delaying making them public until they have been battle-tested by internal usage for a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E2E tests mirror what the customer using the library would write the handler methods. That's why I still prefer keeping the methods public
...Microsoft.Test.E2E.AspNet.OData/Build.AspNetCore3x/BulkOperation/EFTests/BulkInsertTestEF.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
gathogojr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The methods in ODataPathHelper and ODataPathExtensions classes need thorough tests covering all applicable scenarios including but not limited to those I highlighted in my review comments. For example, one can initialize ODataPath with an empty list and most of those methods don't handle such an edge case
| /// <summary> | ||
| /// Extensions method for <see cref="ODataPath"/>. | ||
| /// </summary> | ||
| public static class ODataPathExtensions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, we should keep them internal unless we have concrete use cases in mind where we know customers will need them for, or customers have explicitly asked for them. Making things public increases the maintenance overhead in my opinion. It makes internal improvements and refactoring difficult and changes to their signature almost impossible. But if you still think they should be public, I'm not opposed to that. I but I would suggest considering delaying making them public until they have been battle-tested by internal usage for a while.
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.AspNet.OData.Shared/Extensions/ODataPathExtensions.cs
Outdated
Show resolved
Hide resolved
|
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
Issues
This pull request fixes #xxx.
Description
Since we removed
ODataIdResolverand changed theGetHandlermethod inODataApiHandler, we need to update the tests.Below are the changes made.
POST(BulkInsert) requests useODataApiHandlerinstead ofODataIdResolver.GetHandlermethod to usingODataPathas a parameter. Rewrite the logic to get the handlers and add appropriate extension methods.ODataPathHelperclass with appropriate helper methods.Microsoft.Test.E2E.AspNet.OData.BulkInserttoMicrosoft.Test.E2E.AspNet.OData.BulkOperationsince tests are for bothBulkInsertandBulkUpdate.E2E.AspNetCoreproject. Previously they were only inE2E.AspNetCore3xproject.Checklist (Uncheck if it is not completed)
Additional work necessary
If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.