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

[Communication] - PhoneNumberAdministrationClient - adding live tests #16962

Merged
merged 6 commits into from
Nov 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Azure.Communication.Pipeline;
using Azure.Core.TestFramework;

namespace Azure.Communication.Administration.Tests
{
public class PhoneNumberAdministrationClientLiveTestBase : RecordedTestBase<PhoneNumberAdministrationClientTestEnvironment>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to have this base class or can we just move its content to PhoneNumberAdministrationClientLiveTests?

There is only one implementation of CreateClient, so I'm not sure which benefit a base class is giving us.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are more test classes to be created (at least Sample2_PhoneNumberAdministrationClient) so we reuse CreateClient as well as constructor setting Sanitizer and proper TestEnvironment classes. This also goes along with other SDKs test classes have similar base class.

{
public PhoneNumberAdministrationClientLiveTestBase(bool isAsync) : base(isAsync)
=> Sanitizer = new CommunicationRecordedTestSanitizer();

/// <summary>
/// Creates a <see cref="PhoneNumberAdministrationClient" /> with the connectionstring via environment
/// variables and instruments it to make use of the Azure Core Test Framework functionalities.
/// </summary>
/// <returns>The instrumented <see cref="PhoneNumberAdministrationClient" />.</returns>
protected PhoneNumberAdministrationClient CreateClient(bool isInstrumented = true)
{
var client = new PhoneNumberAdministrationClient(
TestEnvironment.ConnectionString,
InstrumentClientOptions(new PhoneNumberAdministrationClientOptions()));

return isInstrumented ? InstrumentClient(client) : client;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using Azure.Core.TestFramework;

namespace Azure.Communication.Administration.Tests
{
public class PhoneNumberAdministrationClientTestEnvironment : TestEnvironment
{
public PhoneNumberAdministrationClientTestEnvironment() : base("communication")
{
}

private const string ConnectionStringEnvironmentVariableName = "COMMUNICATION_CONNECTION_STRING";

public string ConnectionString => GetRecordedVariable(ConnectionStringEnvironmentVariableName);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net.WebSockets;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading.Tasks;
using Azure.Communication.Administration.Models;
using Azure.Core.TestFramework;
using NUnit.Framework;

namespace Azure.Communication.Administration.Tests
{
/// <summary>
/// The suite of tests for the <see cref="PhoneNumberAdministrationClient"/> class.
/// </summary>
/// <remarks>
/// These tests have a dependency on live Azure services and may incur costs for the associated
/// Azure subscription.
/// </remarks>
public class PhoneNumberAdministrationClientLiveTests : PhoneNumberAdministrationClientLiveTestBase
{
/// <summary>
/// Initializes a new instance of the <see cref="PhoneNumberAdministrationClientLiveTests"/> class.
/// </summary>
/// <param name="isAsync">A flag used by the Azure Core Test Framework to differentiate between tests for asynchronous and synchronous methods.</param>
public PhoneNumberAdministrationClientLiveTests(bool isAsync) : base(isAsync)
{
}

[Test]
[TestCase(null, TestName = "GetAllSupportedCountries")]
[TestCase("en-US", TestName = "GetAllSupportedCountriesEnUsLocale")]
public async Task GetAllSupportedCountries(string? locale)
{
var client = CreateClient();

var countries = client.GetAllSupportedCountriesAsync(locale);

await foreach (var country in countries)
{
Assert.IsFalse(string.IsNullOrEmpty(country.CountryCode));
Assert.IsFalse(string.IsNullOrEmpty(country.LocalizedName));
}
}

[Test]
public async Task GetAllPhoneNumbers()
{
var client = CreateClient();

var numbersPagable = client.GetAllPhoneNumbersAsync();
var numbers = await numbersPagable.ToEnumerableAsync();

Assert.IsNotNull(numbers);
Assert.IsNotEmpty(numbers);
}

[Test]
[TestCase(null, null)]
[TestCase("en-US", null)]
[TestCase("en-US", false)]
[TestCase("en-US", true)]
Copy link
Member

Choose a reason for hiding this comment

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

Would different casing like "en-us" fail the request?
Maybe it makes sense to add this as a case.

I don't want to add "fr-fr" or other yet because I think the service will fail as of today but soon it will succeed and fallback to English.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, at this moment if use not supported locale server side produces error.

public async Task GetPlanGroups(string? locale, bool? includeRateInformation)
{
var client = CreateClient();
var countryCode = "US";

var pageablePhonePlanGroups = client.GetPhonePlanGroupsAsync(countryCode, locale, includeRateInformation);

var phonePlanGroups = await pageablePhonePlanGroups.ToEnumerableAsync();

Assert.IsNotNull(phonePlanGroups);
Assert.AreEqual(3, phonePlanGroups.Count);

var firstGroup = phonePlanGroups.First(group => group.PhoneNumberType == PhoneNumberType.Geographic);

Assert.IsNotNull(firstGroup.LocalizedName);
Assert.IsNotNull(firstGroup.LocalizedDescription);

if (includeRateInformation == true)
{
Assert.IsNotNull(firstGroup.RateInformation);
}
else
{
Assert.IsNull(firstGroup.RateInformation);
}
}

[Test]
[TestCase(null)]
[TestCase("en-US")]
public async Task GetPhonePlans(string? locale)
{
var client = CreateClient();
var countryCode = "US";

var pageablePhonePlanGroups = client.GetPhonePlanGroupsAsync(countryCode, locale);
var phonePlanGroups = await pageablePhonePlanGroups.ToEnumerableAsync().ConfigureAwait(false);

var pageablePhonePlans = client.GetPhonePlansAsync(countryCode, phonePlanGroups.First().PhonePlanGroupId, locale);
var phonePlans = await pageablePhonePlans.ToEnumerableAsync();

Assert.IsNotNull(phonePlans);
Assert.IsNotEmpty(phonePlans);
}

[Test]
[TestCase(null)]
[TestCase("en-US")]
public async Task GetAreaCodesForPlan(string? locale)
{
var client = CreateClient();
var countryCode = "US";

var pageablePhonePlanGroups = client.GetPhonePlanGroupsAsync(countryCode, locale);
var phonePlanGroups = await pageablePhonePlanGroups.ToEnumerableAsync().ConfigureAwait(false);

string phonePlanGroupId = phonePlanGroups.First(group => group.PhoneNumberType == PhoneNumberType.Geographic).PhonePlanGroupId;
var pageablePhonePlans = client.GetPhonePlansAsync(countryCode, phonePlanGroupId, locale);
var phonePlans = await pageablePhonePlans.ToEnumerableAsync();
var phonePlanId = phonePlans.First().PhonePlanId;

var locationOptionsResponse = await client.GetPhonePlanLocationOptionsAsync(countryCode, phonePlanGroupId, phonePlanId).ConfigureAwait(false);

var locationOptions = new List<LocationOptionsQuery>
{
new LocationOptionsQuery
{
LabelId = "state",
OptionsValue = "NY"
},
new LocationOptionsQuery
{
LabelId = "city",
OptionsValue = "NOAM-US-NY-NY"
}
};

var areaCodes = await client.GetAllAreaCodesAsync("selection", countryCode, phonePlanId, locationOptions);

Assert.IsNotNull(areaCodes.Value.PrimaryAreaCodes);
Assert.IsNotEmpty(areaCodes.Value.PrimaryAreaCodes);
}

[Test]
[TestCase(null)]
[TestCase("en-US")]
[AsyncOnly]
public async Task CreateReservationErrorState(string? locale)
{
var client = CreateClient();
var countryCode = "US";

var pageablePhonePlanGroups = client.GetPhonePlanGroupsAsync(countryCode, locale);
var phonePlanGroups = await pageablePhonePlanGroups.ToEnumerableAsync().ConfigureAwait(false);

string phonePlanGroupId = phonePlanGroups.First(group => group.PhoneNumberType == PhoneNumberType.TollFree).PhonePlanGroupId;
var pageablePhonePlans = client.GetPhonePlansAsync(countryCode, phonePlanGroupId, locale);
var phonePlan = (await pageablePhonePlans.ToEnumerableAsync()).First();
var tollFreeAreaCode = phonePlan.AreaCodes.First();

string geographicPhonePlanGroupId = phonePlanGroups.First(group => group.PhoneNumberType == PhoneNumberType.Geographic).PhonePlanGroupId;
var geographicPhonePlanId = (await client.GetPhonePlansAsync(countryCode, geographicPhonePlanGroupId, locale).ToEnumerableAsync()).First().PhonePlanId;

var reservationOptions = new CreateReservationOptions("My reservation", "my description", new[] { geographicPhonePlanId }, tollFreeAreaCode);
reservationOptions.Quantity = 1;
var reservationOperation = await client.StartReservationAsync(reservationOptions);

try
{
await reservationOperation.WaitForCompletionAsync().ConfigureAwait(false);
}
catch (Exception ex)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not following. Why do we expect this to fail and how would this test continue past line 182?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have made a recording of operation that became an error state. This is the case to test we handling error state properly. There are sore asserts after the catch that are redundant. I will remove them

{
Assert.AreEqual("Reservation has failed.", ex.Message);
return;
Copy link
Member

Choose a reason for hiding this comment

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

If I understand you correctly, this test case only fails as expected because the recorded response has an error in it.
The code however isn't guranteed to produce an error case by itself, and it made me as the reader wonder what's supposedly wrong with it.

The problem with this approach is that recordings can no longer be regenerated and do the same thing because they rely on a server condition that we can't control.

Can we change the code instead to reliably produce an error state?

}

Assert.Fail("WaitForCompletionAsync should have thrown an exception.");
}

[Test]
[TestCase(null)]
[TestCase("en-US")]
[AsyncOnly]
public async Task CreateReservation(string? locale)
Copy link
Member

Choose a reason for hiding this comment

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

The recordings for CreateReservationErrorState don't show any non-200 status codes. Maybe CreateReservationErrorState isn't completely implemented and just an early copy paste? :-)

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 do not have any non 200 status codes to take care of. Those statuses are handled in the generated code.
When reservation is failing we receive a reservation object with 200 http status, but payload has a Reservation.Status == error and that case is tested in CreateReservationErrorState

{
var client = CreateClient();
var countryCode = "US";

var pageablePhonePlanGroups = client.GetPhonePlanGroupsAsync(countryCode, locale);
var phonePlanGroups = await pageablePhonePlanGroups.ToEnumerableAsync().ConfigureAwait(false);

string phonePlanGroupId = phonePlanGroups.First(group => group.PhoneNumberType == PhoneNumberType.TollFree).PhonePlanGroupId;
var pageablePhonePlans = client.GetPhonePlansAsync(countryCode, phonePlanGroupId, locale);
var phonePlan = (await pageablePhonePlans.ToEnumerableAsync()).First();
var areaCode = phonePlan.AreaCodes.First();

var reservationOptions = new CreateReservationOptions("My reservation", "my description", new[] { phonePlan.PhonePlanId }, areaCode);
reservationOptions.Quantity = 1;
var reservationOperation = await client.StartReservationAsync(reservationOptions);

await reservationOperation.WaitForCompletionAsync().ConfigureAwait(false);

Assert.IsNotNull(reservationOperation);
Assert.IsTrue(reservationOperation.HasCompleted);
Assert.IsTrue(reservationOperation.HasValue);

var reservation = reservationOperation.Value;
Assert.IsNotNull(reservation);

Assert.AreEqual(ReservationStatus.Reserved, reservation.Status);
Assert.AreEqual(areaCode, reservation.AreaCode);
Assert.IsNull(reservation.ErrorCode);
Assert.AreEqual(1, reservation.PhoneNumbers?.Count);
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

In JS we've added tests for Purchase and Release that we only run when an environment variable INCLUDE_PHONENUMBER_LIVE_TESTS is set to true or when we are in Playback mode.

It would be good to implement them for .NET. If you think it's worth a separate PR that makes sense too.

Here is an example where we test and skip early for Purchase
https://github.com/Azure/azure-sdk-for-js/blob/1605209a1d72dab78f83f53211e33aee938c9f2c/sdk/communication/communication-administration/test/lro.purchase.spec.ts#L82

it("can wait until a reservation is purchased", async function() {
    if (!includePhoneNumberLiveTests && !isPlaybackMode()) {
      this.skip();
    }

Loading