-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[MetricsAdvisor] Made NotificationHook abstract #22345
Conversation
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/src/Models/NotificationHook/NotificationHook.cs
Show resolved
Hide resolved
public SubstringConstraint ContainsJsonString(string propertyName, string propertyValue) => | ||
Contains.Substring($"\"{propertyName}\":\"{propertyValue}\""); | ||
|
||
// Currently only supports a single-element array. | ||
public SubstringConstraint ContainsJsonStringArray(string propertyName, string elementValue) => | ||
Contains.Substring($"\"{propertyName}\":[\"{elementValue}\"]"); |
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.
Convenience methods to make Asserts prettier (examples in tests).
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/tests/Models/NotificationHookModelTests.cs
Show resolved
Hide resolved
sdk/metricsadvisor/Azure.AI.MetricsAdvisor/tests/Models/NotificationHookModelTests.cs
Show resolved
Hide resolved
MockRequest request = mockTransport.Requests.Last(); | ||
string content = ReadContent(request); | ||
|
||
Assert.That(request.Uri.Path, Is.EqualTo($"/metricsadvisor/v1.0/hooks/{FakeGuid}")); |
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.
do you really need to validate the uri? I didn't see anything in your code that will modify this.
Also, consider making the version match the version the user sets in the client so you don't have to update this test every time you update the service version.
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're checking that we could get and send back every NotificationHook
property. We're doing it for the URI as well because that's the only way of validating the Id
property (we don't send it as part of the JSON body).
You have a point about the version. I'll update the assertion to a Contains.Substring(FakeGuid)
given that we just want to test if the ID is being sent correctly.
Making
NotificationHook
abstract after architect feedback.Similar changes are expected for
MetricFeedback
,DataFeedSource
, andDataSourceCredentialEntity
.