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] Add tests for extra Get call in the Create methods #18589

Closed
kinelski opened this issue Feb 9, 2021 · 3 comments
Closed

[MetricsAdvisor] Add tests for extra Get call in the Create methods #18589

kinelski opened this issue Feb 9, 2021 · 3 comments
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor test-enhancement
Milestone

Comments

@kinelski
Copy link
Member

kinelski commented Feb 9, 2021

We need to test the behavior when the first call (create) succeeds and the last call (get) fails. We can probably do it by mocking our client.

Methods to test:

  • CreateDataFeed
  • CreateDetectionConfiguration
  • CreateAlertConfiguration
  • CreateHook
  • AddFeedback

Note: maybe only applicable to CreateDataFeed depending on the outcome of #18480.

@kinelski kinelski added Test Debt Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor labels Feb 9, 2021
@kinelski kinelski added this to the Backlog milestone Feb 9, 2021
@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Mar 12, 2021
@ghost
Copy link

ghost commented Mar 12, 2021

Hi @kinelski. There hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by removing the no-recent-activity label. Otherwise, we'll close this out in 7 days.

@kinelski kinelski removed the no-recent-activity There has been no recent activity on this issue. label Mar 12, 2021
@ghost
Copy link

ghost commented Mar 13, 2021

Hi @kinelski. There was a mistake and this issue was unintentionally flagged as a stale pull request. The label has been removed and the issue will remain active; no action is needed on your part. Apologies for the inconvenience.

@kinelski kinelski modified the milestones: Backlog, [2021] May Apr 19, 2021
@jsquire jsquire added test-reliability Issue that causes tests to be unreliable and removed Test Debt labels Jul 16, 2021
@kinelski kinelski added test-enhancement and removed test-reliability Issue that causes tests to be unreliable labels Aug 11, 2021
@ibocon
Copy link
Contributor

ibocon commented Sep 26, 2021

Hi, @kinelski. I am interested in this issue.

I see this issue is started from your comment in PR #18491. And #18480 does not avoid using extra Get method. So Methods to test are

  • CreateDataFeed
  • CreateDetectionConfiguration
  • CreateAlertConfiguration
  • CreateHook
  • AddFeedback

I am planning to create a mock of MetricsAdvisorAdministrationClient like below as you suggest in description,

public class MockMetricsAdvisorAdministrationClient : MetricsAdvisorAdministrationClient
{
        public virtual async Task<Response<AnomalyAlertConfiguration>> GetAlertConfiguration(string alertConfigurationId, CancellationToken cancellationToken = default)
        {
            // Throw exception when CreateAlertConfiguration call GetAlertConfiguration inside to test.
            throw;
        }
        
        // Other Get method mocks...
}

Where should I put this class in project Azure.AI.MetricsAdvisor.Tests?


LiveTests create MetricsAdvisorAdministrationClient instance form MetricsAdvisorLiveTestBase.GetMetricsAdvisorAdministrationClient method like below,

MetricsAdvisorAdministrationClient adminClient = GetMetricsAdvisorAdministrationClient(useTokenCredential);

So, should I create new MetricsAdvisorLiveTestBase, which use MockMetricsAdvisorAdministrationClient to create a live tests?

public class MockMetricsAdvisorLiveTestBase : MetricsAdvisorLiveTestBase
{
            public override MetricsAdvisorAdministrationClient GetMetricsAdvisorAdministrationClient(bool useTokenCredential = false)
        {
            return new MockMetricsAdvisorAdministrationClient();
        }
}

Or, just create a method like in AnomalyAlertConfigurationTests?

private MetricsAdvisorAdministrationClient GetMetricsAdvisorAdministrationClient()
{
var fakeEndpoint = new Uri("http://notreal.azure.com");
var fakeCredential = new MetricsAdvisorKeyCredential("fakeSubscriptionKey", "fakeApiKey");
return new MetricsAdvisorAdministrationClient(fakeEndpoint, fakeCredential);
}

private MetricsAdvisorAdministrationClient GetMetricsAdvisorAdministrationClient()
{
    return new MockMetricsAdvisorAdministrationClient();
}

@github-actions github-actions bot locked and limited conversation to collaborators Mar 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Cognitive - Metrics Advisor test-enhancement
Projects
None yet
Development

No branches or pull requests

4 participants