migueleliasweb / go-github-mock

A library to aid unittesting code that uses Golang's Github SDK
MIT License
94 stars 22 forks source link

feat: add WithRateLimit as a new MockBackendOption #59

Closed jlucktay closed 8 months ago

jlucktay commented 8 months ago

Use the rate limiter from golang.org/x/time to provide a new MockBackendOption: WithRateLimit

Resolves migueleliasweb/go-github-mock#57.

migueleliasweb commented 8 months ago

Thanks for the contribution, @jlucktay !

I thought of adding this as a fix for #57 but was unsure this would create instability for the tests as the rate is applied to the whole server and all GH endpoints. Any thoughts on this?

jlucktay commented 8 months ago

I started playing around with a variant on this with another MockBackendOption that has this signature: func WithRateLimitedEndpoints(rps float64, endpoints ...EndpointPattern) MockBackendOption I also thought about embedding EndpointPattern into another struct and adding an RPS field to that, something like RateLimitedEndpointPattern.

Either way, the implementations could cover off one limiter per specified endpoint or one limiter across all specified endpoints.

This PR is one limiter for the whole mock server, which, I think, is a good baseline to work forward from. Larger and more complex tests could definitely benefit from implementing either/both of the above on top of this, for sure.

migueleliasweb commented 8 months ago

That's quite interesting! We basically went through almost the same iterations of ideas :smile: . I do like the idea of adding the rate limiting per endpoint though.

It would allow a more gradual rollout of this without messing with more complex tests that might have been written out there.

What do you think of something amongst the lines of:

mock.WithRequestMatchOptions(
    mock.GetUsersOrgsByUsername,
    []github.Organization{
        {
            Name: github.String("foobar123thisorgwasmocked"),
        },
    },
    &RequestMatchOptions{
        RPS: 1,
    },
)

This would look quite familiar compared to the rest of the api for go-github-mock that solidified in the last year or so. There would also be a possibility of adding more options in the future in the struct.

I assume this is somewhat close to what you said:

I also thought about embedding EndpointPattern into another struct and adding an RPS field to that, something like RateLimitedEndpointPattern.

jlucktay commented 8 months ago

Just pushed two small refactors:

I think this PR is OK to go as-is now, to get some basic rate limiting into the mix. Next I will draft another PR with another branch from this one with some variations/ideas on the per-endpoint implementations. Does that sound OK @migueleliasweb?

migueleliasweb commented 8 months ago

Thanks for the contribution!