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

Fakes Support #16613

Closed
MartinForReal opened this issue Dec 10, 2021 · 56 comments
Closed

Fakes Support #16613

MartinForReal opened this issue Dec 10, 2021 · 56 comments
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. test-enhancement

Comments

@MartinForReal
Copy link
Contributor

I'm wondering if there will be support for mock client because it will help end user develop unit test case.

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 10, 2021
@MartinForReal MartinForReal changed the title Mock client is need in unit test scenario. Mock client is essential in unit test scenario. Dec 10, 2021
@RickWinter RickWinter added Client This issue points to a problem in the data-plane of the library. test-enhancement labels Dec 14, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 14, 2021
@Rohitrajak1807
Copy link

A mock client like in the case of knative package for GoLang would be helpful. They define a reactor(this object holds the logic of how the client would "react" given some input), and a mock client which then requests this reactor for some data.
To be specific: https:/knative/client/blob/main/pkg/serving/v1/client_test.go

@haitch
Copy link
Member

haitch commented Aug 29, 2022

please priority this request, this is blocking AKS from moving to track2 API.

we rely on interface for our unittest too.

@jhendrixMSFT jhendrixMSFT self-assigned this Aug 29, 2022
@serbrech
Copy link
Member

serbrech commented Sep 8, 2022

I really don't think we need interfaces for all clients. these can be written by the consumer.
as long as the sdk allows for testability, there is no blocker.
if you do end up providing interfaces, please don't put them in the same package as the client. I personally wouldn't want these interfaces when I import the client.

what does help testability is some test fakes for some particular constructs.
for example

  • testing policies isn't totally straight forward because you need to fake the "next" handler
  • testing the polling is fine, but does require a bit of gymnastics.

@haitch
Copy link
Member

haitch commented Sep 9, 2022

we definitely need type Poller[T any] struct to have a interface.

and all BeginCreateOrUpdate/BeginDelete return this Interface, instead of concrete type.

@serbrech how do you get around this issue?

@jhendrixMSFT
Copy link
Member

Returning an interface is not versioning tolerant, and we explicitly chose not to do that here for this reason.

Here's an example of how you could create a mock Poller[T] https://gist.github.com/jhendrixMSFT/530e6ddad752cd46ac5075a39ae73030

@haitch
Copy link
Member

haitch commented Sep 12, 2022

  1. for API defined on each resource-provider, yes, it is not versioning tolerant, but you can publish interface per version just as what you do on track 1 sdk.
  2. for Poller, I think publish interface is benefiting both side of us, you would have flexibility to change the implementation, even add more implementation, we can mock it without re-define the sdk method we are invoking.

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Sep 12, 2022

A Poller[T] interface would still not be versioning tolerant. You can't add a new method to an interface in a non-breaking way (yes you can use non-publicly implementable interfaces but it's a code smell). In addition, adding this extra level of indirection in public surface area, specifically for testing purposes (which not everybody uses), is a bit of an anti-pattern.

We can (and do) change the implementation without the need for a public interface.

@serbrech
Copy link
Member

as I said, the only part that felt less test friendly was the poller. but with a little helper, it became quite straight forward.
you can provide your own implementation of a poller, so we just wrote a generic FakePollerHandler that we can configure on our fake clients :

p, _ := runtime.NewPoller(nil, runtime.Pipeline{}, &runtime.NewPollerOptions[v20220602preview.FleetMembershipsClientDeleteResponse]{

	Handler: &FakePollerHandler[v20220602preview.FleetMembershipsClientDeleteResponse]{
		Res: f.PollerResponse.BeginDeleteResponse,
		Err: f.PollerResponse.Err,
		
	},
})

@serbrech
Copy link
Member

The gist from @jhendrixMSFT also works by decorating the real client. it's probably simpler than our own setup in fact.

@serbrech
Copy link
Member

No matter how you go, the interface changes completely between track1 and track2, so in the scope of a migration from track1 to track2, asking for the interfaces in track 2 does not reduce the work of the migration at all.

you might as well generate the minimal interface you need yourself. That will ensure that you control how they look, and you are still free to put an adapter in between if you want a different api for your service. It also doesn't impose this unnecessary requirement on the SDK

@serbrech
Copy link
Member

serbrech commented Sep 13, 2022

I do think that the SDK team should spend time writing client tests as an exercise, and documenting how they expect the end user to fake the clients. When I search the SDK repo for how you test the functionality, I found many occurences where your tests would instanciate an internal struct that wouldn't be accessible to me. (the poller for example).

That would force you to think about testability from the end user perspective. Maybe it will drive some API changes, or drive the creation of a test helper package.

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Sep 13, 2022

We've done this to some degree, and it did end up slightly changing some of the APIs (earlier iterations of the Poller weren't mocking friendly at all).

For mocking, there are various tools floating around can be used to generate interfaces/stubs, so we didn't feel the need to generate them ourselves (there's also the whole "don't overuse mocks" so we wanted to explore other options). I can put together a doc/sample of how this would work.

Internally, we've been using recordings which works well but comes with its own baggage. We also haven't productized our recording framework at present, though there have been some asks for this.

I've also been experimenting with how we could achieve a fake, similar to what GCP offers (example). I'm hacking on a rough prototype to see if it has any merit. The nice thing is that this doesn't require any changes to the client; you just plug in the fake via the client options.

client, err := armresources.NewClient("subID", &fakeTokenCredential{}, &arm.ClientOptions{
	ClientOptions: azcore.ClientOptions{
		Transport: NewFakeARMResourcesServer(&myFakeARMResourcesServer{}), // magic happens here
	},
})

The fakes would likely be codegen'ed but live elsewhere (sub-package, or possibly separate repo?).

@haitch
Copy link
Member

haitch commented Sep 13, 2022

fake/mock on transport layer would require us to carry raw request payload.

while we prefer fake/mock on client layer, so we can just mock/fake the object directly.

@jhendrixMSFT
Copy link
Member

It doesn't have to, and my current prototype doesn't require raw payloads.

@jhendrixMSFT
Copy link
Member

This is my fake server proof-of-concept.

https://gist.github.com/jhendrixMSFT/efd5bcc94f2d545b5cb3a4f5e66446f4

It fakes three APIs from armresources.

@serbrech
Copy link
Member

I like this direction @jhendrixMSFT. you provide testability without compromising the API, and the consumer still have all the flexibility they want.

As a consumer, if you're ok generating mocks for your tests, you might as well generate the interfaces too.

@prashantrakheja
Copy link

@jhendrixMSFT any tentative timeline when fakes would be available?

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Nov 16, 2022

We've been performing some internal user-studies on a few different designs. And while I think we're close, the final design hasn't landed yet.

The current prototype, which hasn't incorporated feedback from our last round of internal testing, can be found here if you'd like to see the direction we're going.

@Rohitrajak1807
Copy link

We've been performing some internal user-studies on a few different designs. And while I think we're close, the final design hasn't landed yet.

The current prototype, which hasn't incorporated feedback from our last round of internal testing, can be found here if you'd like to see the direction we're going.

Thanks @jhendrixMSFT !

@yevgenypats
Copy link

We've done this to some degree, and it did end up slightly changing some of the APIs (earlier iterations of the Poller weren't mocking friendly at all).

For mocking, there are various tools floating around can be used to generate interfaces/stubs, so we didn't feel the need to generate them ourselves (there's also the whole "don't overuse mocks" so we wanted to explore other options). I can put together a doc/sample of how this would work.

Internally, we've been using recordings which works well but comes with its own baggage. We also haven't productized our recording framework at present, though there have been some asks for this.

I've also been experimenting with how we could achieve a fake, similar to what GCP offers (example). I'm hacking on a rough prototype to see if it has any merit. The nice thing is that this doesn't require any changes to the client; you just plug in the fake via the client options.

client, err := armresources.NewClient("subID", &fakeTokenCredential{}, &arm.ClientOptions{
	ClientOptions: azcore.ClientOptions{
		Transport: NewFakeARMResourcesServer(&myFakeARMResourcesServer{}), // magic happens here
	},
})

The fakes would likely be codegen'ed but live elsewhere (sub-package, or possibly separate repo?).

@jhendrixMSFT This kind of approach will work nicely but you will need to expose another variable to each New*Client or potentially add the option struct to to be able change the host so it will talk to the test httpserver. See how we do this for GCP here - https:/cloudquery/cloudquery/blob/main/plugins/source/gcp/resources/services/compute/addresses_mock_test.go#L43

We are heavy users of mock/unit-test for all the big cloud providers so this will be super valuable to us instead of writing interfaces manually for the Azure SDK.

@haitch
Copy link
Member

haitch commented Nov 30, 2022

actually we don't care version tolerant for those interface, we only care if it's mock friendly. (assume you publish interface per version, and I don't think you can easily publish interface regulate all versions)

bump version is a manual process with pull-request anyway, we used to update code along version bump.

@jhendrixMSFT
Copy link
Member

@unmarshall thanks for the feedback. Indeed we have a problem here when executing faked pager/poller APIs concurrently. I'll take a look at how we can fix this.

@jhendrixMSFT
Copy link
Member

@unmarshall we've released updated betas with a fix for this.

@unmarshall
Copy link

@jhendrixMSFT I was trying to write tests using fake package for resourcegraph. Since we have more than one queries to test for i was wondering if there is an existing parser for the query string present inside armresourcegraph.QueryRequest. This would then give me a handle to the table and other aspects of the query using which i can then implement the behavior inside the fake server and dish out relevant response. I tried looking at code in this project and also in https:/Azure/azure-kusto-go but could not find anything that given a query string constructs a golang type representing it. Can you provide any hint on how to achieve it?

@jhendrixMSFT
Copy link
Member

@unmarshall unfortunately we don't have any such construct as we only generate client-side content.

@unmarshall
Copy link

@unmarshall unfortunately we don't have any such construct as we only generate client-side content.

Thanks for your response. With the introduction of fakes, do you think such a thing would be helpful to write tests?

@unmarshall
Copy link

unmarshall commented Jul 28, 2023

@jhendrixMSFT I was writing fake implementation for armmarketplaceorder.MarketplaceAgreementsClient and found its behavior is weird. I have now documented my observations in ticket-1 and ticket-2. The current behavior makes the implementation of a fake server a bit complicated.

@unmarshall
Copy link

@jhendrixMSFT Any timeline when fake support will be out of beta?

@jhendrixMSFT
Copy link
Member

@unmarshall we've been conducting some user-studies the past few weeks to see where we can make improvements. Those have concluded, so we're working on incorporating the feedback. I'm hoping that this can go GA in our September release but I'm not 100% sure yet.

@unmarshall
Copy link

Those have concluded, so we're working on incorporating the feedback. I'm hoping that this can go GA in our September release but I'm not 100% sure yet.
@jhendrixMSFT Thanks for a quick update. This sounds encouraging. This tentative timeline works for us.

@unmarshall
Copy link

@jhendrixMSFT Any update on the promotion of fakes support to GA?

@jhendrixMSFT
Copy link
Member

Yes! I meant to post this earlier here but got sidetracked. We're going to GA fakes in our November release window. So, the first week of November we'll release [email protected] with the fake package along with an updated code generator. The following week we'll begin refreshing all of the ARM modules with fakes enabled. Data-plane will come sometime later after we've had a bit of "bake time" to see if there's any feedback we need to incorporate.

@stephengroat
Copy link

thanks so much for this update with general timelines, super helpful for planning the refactor to take advantage of these new features

@serbrech
Copy link
Member

Congrats to you and the SDK team on this accomplishment @jhendrixMSFT! We greatly appreciate the transparency and active engagement! 🎉

@jhendrixMSFT
Copy link
Member

Just a heads-up to folks using fakes that fake.NewTokenCredential has been removed in prep for the v1.9.0 release. It doesn't add any value over using a literal &fake.TokenCredential{}.

@jhendrixMSFT
Copy link
Member

All of the ARM SDKs have been updated with fakes support.

@unmarshall
Copy link

@jhendrixMSFT -sdk/azidentity is still in beta (https:/Azure/azure-sdk-for-go/releases/tag/sdk%2Fazidentity%2Fv1.5.0-beta.2) Any plans to move that out of beta? I am assuming beta version of azidentity has fake support in it, right?

@jhendrixMSFT
Copy link
Member

@unmarshall we haven't considered adding fakes to azidentity. Its main job is to provide credential types that satisfy the azcore.TokenCredential interface. So, we've added azcore/fake.TokenCredential for that purpose. Can we get more info about your scenario?

@sbanic-venafi
Copy link

sbanic-venafi commented Jan 9, 2024

@jhendrixMSFT : Hi, I noticed that the latest SDK has key vault secrets and keys fake clients (in resourcemanager/keyvault/armkeyvault/fake, but the certificates fake client is not present?

@jhendrixMSFT
Copy link
Member

Hmm I don't see a client for certificates in resourcemanager/keyvault/armkeyvault. Can you link to the client in question?

@sbanic-venafi
Copy link

sbanic-venafi commented Jan 9, 2024

So, I may be looking incorrectly. Let's see...
I'm looking to fake 3 clients:

  • security/keyvault/azsecrets/ -- azsecrets.Client
    • for this I see resourcemanager/keyvault/armkeyvault/fake -- fake.NewSecretsServerTransport and fake.SecretsServer
  • security/keyvault/azkeys -- azkeys.Client
    • for this I see resourcemanager/keyvault/armkeyvault/fake -- fake.NewKeysServerTransport and fake.KeysServer
  • security/keyvault/azcertificates -- azcertificates.Client
    • for this I cannot find the equivalent in the resourcemanager/keyvault/armkeyvault/fake

Perhaps I'm not implementing this correctly? I'm just starting at this...

@jhendrixMSFT
Copy link
Member

jhendrixMSFT commented Jan 9, 2024

Thanks for clarifying.

We've only enabled fakes for the ARM SDKs, i.e. the ones under /sdk/resourcemanager. We wanted to give fakes a bit of bake time there before adding them to the other SDKs in case any bugs were encountered that we need to fix. That said, maybe we can get betas out for the key vault SDKs (will follow up with the team about this).

@badass-aoz
Copy link

Thanks for putting together the fake library. It makes testing my codes much easier.

Is there a plan to do the same for azblob as well any time soon? I'm mostly looking at mocking out calls to azblob.Client.

@discentem
Copy link
Contributor

discentem commented Jan 23, 2024

Is there a plan to do the same for azblob as well any time soon? I'm mostly looking at mocking out calls to azblob.Client.

I'd love to see fakes directly provided by azblob but in case it's helpful @badass-aoz this is what I do currently in one of my projects

type azureBlobishClient interface {
	UploadStream(ctx context.Context, containerName string, blobName string, body io.Reader, o *azblob.UploadStreamOptions) (azblob.UploadStreamResponse, error)
	DownloadStream(ctx context.Context, containerName string, blobName string, o *azblob.DownloadStreamOptions) (azblob.DownloadStreamResponse, error)
}

https:/discentem/cavorite/blob/0120a382302419862b3eff1db568128f25a2be72/stores/azure_test.go#L18-L20

Obviously just expand the interface for whatever methods you need to use in production code path.

@badass-aoz
Copy link

Thanks @discentem ! I laugh-cried at your message because that's exactly what I had for my project in lieu of an official fake support. This felt a bit too hacky hence if there's a timeline from @jhendrixMSFT then I'd rather wait for that to happen first.

@jhendrixMSFT
Copy link
Member

Unfortunately, I don't have a good timeline for fakes in azblob (I will circle back with my team about this). In the meantime, using interfaces as described earlier is your best bet (assuming you don't want to use the storage emulator).

Copy link

Hi @MartinForReal, we deeply appreciate your input into this project. Regrettably, this issue has remained inactive for over 2 years, leading us to the decision to close it. We've implemented this policy to maintain the relevance of our issue queue and facilitate easier navigation for new contributors. If you still believe this topic requires attention, please feel free to create a new issue, referencing this one. Thank you for your understanding and ongoing support.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 13, 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. test-enhancement
Projects
None yet
Development

No branches or pull requests