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

[MetricsAdvisor] Added tests for the AnomalyDetectionConfiguration CRUD operations #17590

Merged
merged 7 commits into from
Dec 21, 2020

Conversation

kinelski
Copy link
Member

Fixes #15915.

Tests ended up very large because we're testing a complex model. It takes multiple lines for setup and some more for validation.

@kinelski kinelski added Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor labels Dec 16, 2020
@kinelski kinelski self-assigned this Dec 16, 2020
{
MetricsAdvisorAdministrationClient adminClient = GetMetricsAdvisorAdministrationClient();

var config = new AnomalyDetectionConfiguration(FakeGuid, "configName", new ());
Copy link
Member

Choose a reason for hiding this comment

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

just curious, is there any test that will verify the required input for AnomalyDetectionConfiguration?

Copy link
Member Author

@kinelski kinelski Dec 18, 2020

Choose a reason for hiding this comment

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

There isn't. However, we'll change some of our models' designs to have parameterless constructors (including AnomalyDetectionConfiguration), so these tests would be discarded. This is feedback we got during the archboard review (#16321).

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll still have models that may need validation after that, though. That's a good point, so just opened #17650 to track this. We can work on that after the design change.

Copy link
Member

@maririos maririos left a comment

Choose a reason for hiding this comment

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

LGTM. It will be good if someone with full understanding of the service (from other language?) will check as I am not super confident with all configurations

@kinelski
Copy link
Member Author

LGTM. It will be good if someone with full understanding of the service (from other language?) will check as I am not super confident with all configurations

Just added others as reviewers in case they want to contribute. I'll keep it open for more time.

Copy link
Member

@kristapratico kristapratico left a comment

Choose a reason for hiding this comment

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

looks good!

@kinelski kinelski merged commit 4463cc2 into Azure:master Dec 21, 2020
@kinelski kinelski deleted the ma-detect branch December 21, 2020 20:16
annelo-msft pushed a commit to annelo-msft/azure-sdk-for-net that referenced this pull request Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MetricsAdvisor] Write tests for the AnomalyDetectionConfiguration CRUD operations
3 participants