Skip to content

Make extension clients public to enable mocking for extension methods#3483

Merged
ArcturusZhang merged 68 commits intoAzure:feature/v3from
ArcturusZhang:make-extensionclients-public
Nov 2, 2023
Merged

Make extension clients public to enable mocking for extension methods#3483
ArcturusZhang merged 68 commits intoAzure:feature/v3from
ArcturusZhang:make-extensionclients-public

Conversation

@ArcturusZhang
Copy link
Member

@ArcturusZhang ArcturusZhang commented Jun 8, 2023

Fixes #3493

Description

This is a simplified version of #3175

This PR enables a general way for our customer to mock the extension methods.
In general, to mock an extension method in out .Net mgmt SDK takes several unified steps of mocking:

  1. Identify the type that the extension method is extending (referred as extendee below), follow the pattern Mockable{RPName}{ExtendeeName}, you will get a type which contains the real method to mock (referred as extension class below). For instance if the extendee is ResourceGroupResource in Azure.ResourceManager.Compute, you will get MockableComputeResourceGroupResource.
  2. Mock the method on the extension class with the same parameter list as the extension method you would like to mock. For instance, to mock the extension method public static Response DoSomething(this ResourceGroupResource resourceGroup, string someParameter, CancellationToken cancellationToken), you should mock the method public virtual Response DoSomething(string someParameter, CancellationToken cancellationToken) on the extension class MockableComputeResourceGroupResource.
  3. Mock a hidden method GetCachedClient on the extendee to make it return the above mocked extension class.

By following the above three steps, your mock now should work in the same way that you usually call it.

This PR unifies the previous internal implementation to ensure we have the same pattern everywhere so that our customer gets a unified experience of mocking the extension methods.

This PR also tries to remove all the validations in the public extension class to make mocking more feasible. For instance, previously we have the code:

public static partial class ComputeExtensions
{
	public static Response DoSomething(this ResourceGroupResource resourceGroup, string name)
	{
		Argument.ThrowIfNull(name, nameof(name));
		// implementation
	}
}

This will cause the mocking invocation has to pass in a non-null argument as well - but when we are mocking those instance method, since the implementation of the body has been completely replaced, we never have this issue.
Therefore this PR removes all these validations in the static class. But we still enforces those parameter to be non-null or non-empty because the same check will exist in the corresponding Mocking Extensions.

A follow up issue of this is to make a test or analyzer rule to enforce the pattern in all of our SDKs including the generated code and customized code. Because changing the internal implementation here will change the way of mocking things, changing it would be regarded as a behavioral breaking change and we should never change that once the extension classes are public.

Checklist

To ensure a quick review and merge, please ensure:

  • The PR has a understandable title and description explaining the why and what.
  • The PR is opened in draft if not ready for review yet.
    • If opened in draft, please allocate sufficient time (24 hours) after moving out of draft for review
  • The branch is recent enough to not have merge conflicts upon creation.

Ready to Land?

  • Build is completely green
    • Submissions with test failures require tracking issue and approval of a CODEOWNER
  • At least one +1 review by a CODEOWNER
  • All -1 reviews are confirmed resolved by the reviewer
    • Override/Marking reviews stale must be discussed with CODEOWNERS first

@ArcturusZhang ArcturusZhang added the Mgmt This issue is related to a management-plane library. label Jun 8, 2023
@ArcturusZhang ArcturusZhang marked this pull request as ready for review June 9, 2023 08:38
@ArcturusZhang

This comment was marked as outdated.

@ArcturusZhang

This comment was marked as resolved.

@ArcturusZhang

This comment was marked as outdated.

@ArcturusZhang ArcturusZhang force-pushed the make-extensionclients-public branch from 93168fe to 54aef20 Compare July 25, 2023 06:12
@ArcturusZhang
Copy link
Member Author

@m-nash this PR is ready to review.

I have an idea that since we need to collection feedback for some customers, we might cannot merge this PR in a short time.
But in the meantime, the only change that has no turning back is making those mocking extensions public.
How about I separate the "making them public" part out of this PR, and we merge this PR? Because this PR changes quite a few places it would be quite easy to get conflict with a high maintenance cost.

In summary, my plan is to take two steps:

  1. this PR changes everything we have to change for extension method mocking except making those classes public.
  2. open another PR to make those classes public, and this PR will not merge until the beta testing is finished.

@ArcturusZhang ArcturusZhang changed the title Make extension clients public to enable mocking for extension methods Change the pattern of extension methods for mocking Jul 28, 2023
@ArcturusZhang

This comment was marked as resolved.

@ArcturusZhang ArcturusZhang merged commit 22fd260 into Azure:feature/v3 Nov 2, 2023
@ArcturusZhang ArcturusZhang deleted the make-extensionclients-public branch November 2, 2023 09:02
live1206 pushed a commit to live1206/autorest.csharp that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Mgmt This issue is related to a management-plane library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable mocking for extension methods

3 participants

Comments