Skip to content

[MetricsAdvisor] Fixing test failures #22555

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.

Sign up for GitHub

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

Merged
kinelski merged 3 commits into from
Jul 9, 2021
Merged

[MetricsAdvisor] Fixing test failures #22555

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a70690b
Fixed data feed test issues
kinelski Jul 8, 2021
bfff5a4
Fixed hook issue
kinelski Jul 9, 2021
40997bd
Updated sample
kinelski Jul 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...sor/Azure.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/DataFeedLiveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1814,7 +1814,7 @@ private DataFeed GetDataFeedWithOptionalMembersSet(string name, DataFeedSource d
{
Name = name,
DataSource = dataSource,
Granularity = new DataFeedGranularity(DataFeedGranularityType.Custom) { CustomGranularityValue = 1360 },
Granularity = new DataFeedGranularity(DataFeedGranularityType.Custom) { CustomGranularityValue = 3000 },
Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021

Choose a reason for hiding this comment

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

The service added some extra validation when creating a data feed, and was complaining that our granularity value was too low. Increased it and needed to rerecord a lot of tests.

Copy link
Copy Markdown
Member

@maririos maririos Jul 9, 2021

Choose a reason for hiding this comment

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

I imagine this is something the user knows that needs to adjust? is 300 an arbitrary number? or a recommended value from service

Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021

Choose a reason for hiding this comment

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

Completely arbitrary. They need to improve a lot of their error messages.

Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021

Choose a reason for hiding this comment

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

I'll create an issue for this message specifically.

Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021

Choose a reason for hiding this comment

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

#22557

Schema = new DataFeedSchema() { TimestampColumn = "timestamp" },
IngestionSettings = ingestionSettings,
Description = "This data feed was created to test the .NET client.",
Expand Down Expand Up @@ -1957,7 +1957,7 @@ private void ValidateDataFeedWithOptionalMembersSet(DataFeed dataFeed, string ex

Assert.That(dataFeed.Granularity, Is.Not.Null);
Assert.That(dataFeed.Granularity.GranularityType, Is.EqualTo(DataFeedGranularityType.Custom));
Assert.That(dataFeed.Granularity.CustomGranularityValue, Is.EqualTo(1360));
Assert.That(dataFeed.Granularity.CustomGranularityValue, Is.EqualTo(3000));

Assert.That(dataFeed.Schema, Is.Not.Null);
Assert.That(dataFeed.Schema.MetricColumns, Is.Not.Null);
Expand Down
4 changes: 0 additions & 4 deletions ...e.AI.MetricsAdvisor/tests/MetricsAdvisorAdministrationClient/NotificationHookLiveTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,10 +453,6 @@ public async Task GetHooksWithMinimumSetup(bool useTokenCredential)
var webHook = hook as WebNotificationHook;

Assert.That(webHook, Is.Not.Null);
Assert.That(webHook.CertificateKey, Is.Not.Null);
Assert.That(webHook.CertificatePassword, Is.Not.Null);
Assert.That(webHook.Username, Is.Not.Null);
Assert.That(webHook.Password, Is.Not.Null);
Comment on lines 455 to -459
Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021

Choose a reason for hiding this comment

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

The service behaves incredibly weirdly here. Usually they default strings not specified by the user as empty strings. In a webhook, this is a bit different. If not specified, the properties will be undefined, so they won't be returned. If you specify them at least once, this changes forever. If you set one of them to null to reset to their default state, they will be reset to empty strings.

Since we can't guarantee they won't be null here (we're getting a list of random hooks from the resource), removed the checks.

Copy link
Copy Markdown
Member

@maririos maririos Jul 9, 2021

Choose a reason for hiding this comment

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

sounds good for the test. I don't think it is worth adding this to the docstrings. do u?

Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It seems more like accidental behavior than something they actually wanted to do. But I don't think it's really worth it. Users will be setting properties to specific values (Property = new_value), or resetting them to the default value (Property = null), but they won't really be default-checking them.

Assert.That(webHook.Headers, Is.Not.Null);
Assert.That(webHook.Headers.Values.Any(value => value == null), Is.False);
}
Expand Down
4 changes: 2 additions & 2 deletions ...ricsadvisor/Azure.AI.MetricsAdvisor/tests/Samples/Sample02_DataFeedIngestionOperations.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ public async Task RefreshDataFeedIngestionAsync()

string dataFeedId = DataFeedId;

var startsOn = DateTimeOffset.Parse("2020-08-01T00:00:00Z");
var endsOn = DateTimeOffset.Parse("2020-08-03T00:00:00Z");
var startsOn = DateTimeOffset.Parse("2021-06-01T00:00:00Z");
var endsOn = DateTimeOffset.Parse("2021-06-03T00:00:00Z");
Comment on lines -92 to +93
Copy link
Copy Markdown
Contributor Author

@kinelski kinelski Jul 9, 2021

Choose a reason for hiding this comment

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

New validation by the service. They were complaining that we were trying to re-ingest data from before the ingestion start time we had set up for this data feed. Not gonna lie, they have the right to be mad.


await adminClient.RefreshDataFeedIngestionAsync(dataFeedId, startsOn, endsOn);
}
Expand Down
145 changes: 29 additions & 116 deletions ...ds/DataFeedLiveTests/CreateAndGetAzureApplicationInsightsDataFeedWithOptionalMembers.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading