-
Notifications
You must be signed in to change notification settings - Fork 473
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
Issue #1645 Interface Support Mock Testing #2215
base: master
Are you sure you want to change the base?
Conversation
…he injection occurs
Dev/issue 1645
OData#1645 unit test added and all unit tests passing
…ings base on vs studio.
Base line file test\UnitTest\Microsoft.AspNet.OData.Test\PublicApi\Microsoft.AspNet.OData.PublicApi.bsl and output file D:\a\1\s\bin\Release\UnitTest\AspNet\Microsoft.AspNet.OData.PublicApi.out do not match, please check. copy /y "D:\a\1\s\bin\Release\UnitTest\AspNet\Microsoft.AspNet.OData.PublicApi.out" "test\UnitTest\Microsoft.AspNet.OData.Test\PublicApi\Microsoft.AspNet.OData.PublicApi.bsl" I am going to need some help on your side to address this issue. one thing I can try, may not work though [xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.2+2d84eb3141 (32-bit Desktop .NET 4.0.30319.42000) build.cmd quick is not even executing the AspNet 4.5.2 framework version of the testing. the key was to set the processor architecture from auto to x64 for any cpu |
src/Microsoft.AspNet.OData.Shared/Query/Validators/ODataQueryValidator.cs
Show resolved
Hide resolved
.../Microsoft.AspNet.OData.Test.Shared/Formatter/SUT/CollectionSerializerCustomersController.cs
Outdated
Show resolved
Hide resolved
@kccarter76 Thank you for your contribution, Kindly address the above comments. |
I do not have write access to the destination branch. the conflicts that are currently listed are a product of natural development and where not present at the time this was completed. I may not have the time available to address. |
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.
Looks good to me, I didn't find any apparent issues with backwards compatibility.
@kccarter76 Do let me know when you are ready to get this merged in and any help you require.
} | ||
} | ||
|
||
namespace Microsoft.AspNet.OData.Test.Formatter.Interface |
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.
Split this to two namespaces for consistency.
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.
not sure about what you are asking here? been awhile since looking at this code.
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 think you are asking about the sut class version in a class namespace and the interface implementation in an interface namespace. is that correct?
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.
it might be best to split into two separate files to simplify reading the code base.
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 need more eyes on this, since I have been unable to resolve the issue with the unit tests not running. @marabooy, I have sent you a collaboration request. I have hit a road block and the work crews appear to be on a permanent coffee break.
to resolve the above conflicts, it would be best to collaborate. I am not sure what needs to be kept and merge what is needed to support the interface. EnableQueryAttribute was straight forward. however the others not so much. |
@kccarter76 the BSL updates can be done by running the respective PublicApi tests and copying over the new file. |
I am missing code for RequestQueryData. I may need to merge/pull the current master into my github fork. okay my EnableQueryAttribute is really out of sync with the base master and when I try to create a pull request from webApi:master to kccarter76webapi:master. |
…aster # Conflicts: # src/Microsoft.AspNetCore.OData/EnableQueryAttribute.cs # test/UnitTest/Microsoft.AspNet.OData.Test/PublicApi/Microsoft.AspNet.OData.PublicApi.bsl # test/UnitTest/Microsoft.AspNetCore.OData.Test/PublicApi/Microsoft.AspNetCore.OData.PublicApi.bsl # test/UnitTest/Microsoft.AspNetCore.OData.Test/PublicApi/Microsoft.AspNetCore3x.OData.PublicApi.bsl
is this being addressed in the AspNet solution: Severity Code Description Project File Line Suppression State this exception is preventing me from updating 'Microsoft.AspNet.OData.PublicApi.bsl' Severity Code Description Project File Line Suppression State was able to git this worked around by not running code analysis. however, the x unit tests are not running again and the work around from before is not working. |
@@ -2354,9 +2354,48 @@ public enum Microsoft.AspNet.OData.Query.SelectExpandType : int { | |||
Disabled = 2 | |||
} | |||
|
|||
<<<<<<< HEAD |
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.
Kindly fix the merge result here. I will check with someone on the team to see what can be done to help.
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.
working on it, currently dealing with an issue where VS 2019 16.8 will not run unit tests on local machine. see tail end of microsoft/vstest#1220
/// </summary> | ||
[ODataQueryParameterBinding] | ||
[NonValidatingParameterBinding] | ||
public interface IODataQueryOptions |
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.
This is more of a discussion than feedback: I came into this review not really knowing what query options were, and saw that the purpose was to convert it to an interface for mocking. I wondered to myself "how many ways really are there to implement a set of options? won't that just be a data type with a bunch of data type properties? there should just be a constructor overload where anything that needs to be mocked can be set directly instead". Seeing this interface, though, I see the many methods that are useful for the query options. I'm wondering, though, if it makes sense to separate those two things out. In this case, I would think the ApplyTo
overloads belong on some other type rather than the options themselves. Do you have any thoughts on this?
Any updates? |
Any updates @kccarter76 ? |
Issues
Feature enhancement resolves #1645
Description
added the ability to use an interface IODataQueryOptions as a parameter on a controller method.
Checklist (Uncheck if it is not completed)
Additional work necessary
feature needs to still be documented and the issue #1645 updated with Docs Needed.