Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.24k stars 4.58k forks source link

Standardize test infrastructure used in Service Bus to match the rest of T2 libraries #23418

Closed JoshLove-msft closed 2 years ago

JoshLove-msft commented 3 years ago

Event Hubs and Service Bus have *Scope classes that are able to create namespaces and storage accounts on the fly. This allows users to run tests from VS without having to run the New-TestResources script. But... we already have the New-TestResources script and test-resources.json files that allow resource creation for a reason (ease of maintenance, consistency across libraries), so we should figure out what the driving force is behind being different for these two libraries.

jsquire commented 3 years ago

I don't believe this is a reasonable goal at present, due to the differences in the messaging libraries as compared to the HTTP-based. Ignoring that our current test infrastructure is heavily biased towards HTTP scenarios, there are still gaps that we cannot address.

For example:

Unless we make a large investment in a recording framework for AMQP, I don't believe the first two can be addressed.

The third is possible and would help to normalize some of the differences in the TestEnvironment classes but likewise would require a non-trivial investment in additional infrastructure beyond what we have today. This has been discussed in the past, and each time the outcome has been the need for this is driven by a small set of non-HTTP services and the cost isn't justified.

jsquire commented 3 years ago

To clarify, I'm not strongly opposed to removing the dynamic creation of namespaces and storage accounts in favor of Test-Resources, so long as there's a way to specify that the RG created is permanent and doesn't get tagged for automatic cleanup.

That would unify the non-ephemeral resource creation without having a negative impact to the dev-test loop. The ephemeral resource management through the scope classes, however, continue to be necessary unless I'm overlooking an offering in our shared infrastructure.

That said, I don't see a good reason to do so, as this infrastructure collaborates with Test Resources if it was used and extends with additional scenarios that Test Resources does not support; it exists, is well-tested, and is useful. I'd like to understand what we think the value would be.

jsquire commented 3 years ago

Self Notes

If we end up removing the ephemeral namesapce and storage account, also remember to:

pakrym commented 3 years ago

The messaging resources cannot be reliably reset to a clean state between tests; this means that we need to create an ephemeral resource (queue, topic, hub, etc) for each test in order to keep them isolated to prevent cascading failures and allow parallelization. Even with Test-Resources use, this continues to be a problem and is what the "Scope" classes exist to solve.

The same is true for any other resources we use for HTTP-based tests, blobs, configuration settings need to be created and deleted per test.

Test-Resources does not have the right infrastructure to allow for an "F5" experience in Visual Studio, where the deployment is run, tests are run, and then the resource group is torn down. This can be worked around using scripts for a local run from the command-line, but falls short when there is a need to debug tests or for contributors who are more comfortable working from an IDE.

The current workflow for all the services is:

  1. Run New-TestResources <service name>
  2. Do development from VS.

Reading https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/eventhub/Azure.Messaging.EventHubs/CONTRIBUTING.md#running-tests to develop EH contributors need:

  1. Run ./live-tests-azure-setup -SubscriptionName "<< YOUR SUBSCRIPTION NAME >>"
  2. Manually copy and set environment variables
  3. Restart VS
  4. Do development from VS.

It seems that New-TestResources provides a more consistent and convenient experience.

so long as there's a way to specify that the RG created is permanent and doesn't get tagged for automatic cleanup.

Why should messaging be specifically exempt from this?

I agree that messaging has a specific requirement to always run live and to create and remove some resources per test but this can be accomplished with a way smaller amount of additional infrastructure than we have now. It's possible we need to promote it to be a part of test framework as well instead of creating more local copies of it.

My goal is to teach developers and external contributors the same development workflow for all the services and drive experience improvements across the board.

jsquire commented 3 years ago

The same is true for any other resources we use for HTTP-based tests, blobs, configuration settings need to be created and deleted per test.

I believe the difference here is that those assets can be managed from within the client library; that is not the case for Event Hubs. That was not the case for Service Bus at the time the infrastructure was written (though it could be adapted now that there is an admin client).

Why should messaging be specifically exempt from this?

Because the live tests are an integral part of the development loop and having an RG that disappears periodically is an impediment that isn't necessary.

It's possible we need to promote it to be a part of test framework as well instead of creating more local copies of it.

I'm good with that; I've had that conversation several times in the past and there was resistance because "it's just one or two services that need it." Again, the primary difference to take into account is that Event Hubs requires the management SDK to manage the ephemeral resources. If I recall, that has been the main sticking point. That is also the case for some others, like ACR.

The current workflow for all the services is:

Run New-TestResources Do development from VS.

Not quite. Those steps assume the contributor has already created an AAD application and granted it the correct permission to execute the test resources script. Or, maybe I'm overlooking recent functionality for Test-Resources where it bootstraps the environment with a local identity that gets seeded into the test environment? The last I remember, that wasn't happening.

The rest of the comparison isn't apples-to-apples; the Event Hubs steps are a one-time setup - which is the entire point. Do it once and then just run tests forever like you would for any other local project.

The non-AAD parts of the EH setup could (and should) be replaced with Test-Resources (assuming they can be deployed permanently).... they didn't exist when we wrote the EH script and there hasn't been justification to spend the time changing something that works and is a one-time setup.

My goal is to teach developers and external contributors the same development workflow for all the services and drive experience improvements across the board.

Great! On that, we are in agreement. To make that work, I believe that we need to do a better job of understanding what contributing is like for external developers and make sure that our onboarding and developer experience makes things easy rather than being a hurdle to overcome.

jsquire commented 3 years ago

The non-AAD parts of the EH setup could (and should) be replaced with Test-Resources (assuming they can be deployed permanently).... they didn't exist when we wrote the EH script and there hasn't been justification to spend the time changing something that works and is a one-time setup.

Self correction; I just was made aware that Test-Resources has been enhanced to create the service principal now. (that's new from the last I looked)

I would agree that this obviates the EH-specific script.

pakrym commented 3 years ago

The rest of the comparison isn't apples-to-apples; the Event Hubs steps are a one-time setup

It is a one-time setup for all other libraries as well.

Because the live tests are an integral part of the development loop and having an RG that disappears periodically is an impediment that isn't necessary.

It is an integral part of any library development. There is a very limited set of changes you can make without having to run live even in HTTP libraries.

Again, the primary difference to take into account is that Event Hubs requires the management SDK to manage the ephemeral resources.

I'm in agreement that some resources would need to be created per test/test run it is true for other services and definitely true for eventhubs. What I'm trying to say is let's have as few custom features as possible and drive improvement across the board for the shared functionality.

To make that work, I believe that we need to do a better job of understanding what contributing is like for external developers and make sure that our onboarding and developer experience makes things easy rather than being a hurdle to overcome.

We are constantly doing this and improving the current experience. That's why Test-Resources doesn't require any additional parameters, supports bicep, doesn't require VS restart etc. If you have any specific features on your mind please file them here or at https://github.com/Azure/azure-sdk-tools

JoshLove-msft commented 3 years ago

Service Bus is mostly standard at this point. The one thing we still need to do is update to use the ServiceBusAdministrationClient rather than the ARM client for creating queues/topics/subscriptions.

jsquire commented 3 years ago

I'm going to descope Event Hubs from this issue in favor of the set of issues that we agreed on; that'll give me better context and detail so that the action needed is actionable. Those will get written up later this week when I start laying out the next month's milestone.

I'll leave it to @JoshLove-msft's discretion whether he wants to continue tracking Service Bus tasks here.

pakrym commented 3 years ago

It would be great if we can get to a point where we don't need ServiceBusTestEnvironment.Instance anymore

jsquire commented 2 years ago

This as been completed; both Event Hubs and Service Bus are using the standard Test Resources infrastructure.