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

Adding simple ability for multiple responses from a test server #594

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

KevinJCross
Copy link

@KevinJCross KevinJCross commented Sep 21, 2022

Ive used this code in our project to test retry behaviour of a client we developed.
I also used the round robin version to stress test some of the client functionality to check for conection leaks and race conditions.

@onsi
Copy link
Owner

onsi commented Sep 22, 2022

hey thanks for this - i'll take a look when i'm at my computer :)

@thediveo
Copy link
Collaborator

@KevinJCross would you mind also provide a few bits of information and example usage for the documentation? -- the https://onsi.github.io/gomega/ one; you already documented the source code.

@onsi
Copy link
Owner

onsi commented Sep 22, 2022

hey @KevinJCross - first off thanks for this. I can see the utility of both of these and the use cases in your examples make sense. A couple of thoughts, though:

RespondWithMultiple seems pretty close to the default behavior of AppendHandlers(). ghttp already supports handling multiple sequential requests - the only difference is that RespondWithMultiple will pin to the last response once the number of requests exceeds the number of handlers. Here too, though, you can configure the server to keep returning a configurable status code for any remaining requests (though not something more elaborate than that, e.g. a JSON payload).

RoundRobinWithMultiple makes sense to me.

What gives me pause, though, is that both of these handlers will only work if used with RouteToHandler. If a user naively does:

server.AppendHandlers(ghttp.RespondWithMultiple(...))

they won't get the expected behavior - since the handler returned by RespondWithMultiple will only be called once.

So I wonder if there's something deeper we need to adjust in ghttp. One thought would be to add server.SetDefaultHandler() as a catch-all that is called repeatedly if the stack of registered handlers (via AppendHandlers) has been exhausted and if no matching route (via RouteToHandler) matches. We'd still have the issue of folks accidentally attaching ghttp.RoundRobinWithMultiple to AppendHandlers but at least there would be an easy change that keeps them in the "stack of routes" modality vs the "mapping routes to handlers" modality.

Does that make sense?

Also - agreed with @thediveo that it would be good to add some additional narrative-style documentation to https://github.com/onsi/gomega/blob/master/docs/index.md

That can be a good place to motivate the usecases that RoundRobinWithMultiple and RespondWithMultiple are solving - and to clarify how they can/can't be used with AppendHandlers and RouteToHandler.

@KevinJCross
Copy link
Author

KevinJCross commented Sep 26, 2022

hi @onsi and @thediveo
So there is documentation how to use this in the test examples which should show up in the godocs. I.E. the function ExampleRespondWithMultiple should show up in the godocs its also a test btw.

The problem is that append handlers and Set default handler does not add a "use case" the normal use case for this is for testing retries and creating a stress test with some problematic behaviour. The adding of the ResponseWithMultiple is obvious in the way it has one set of circular handlers for one single endpoint.

In our case we set up a whole environment of services and responses on the single ghttp server so setting the default response to be repeated could be confusing.

Most of the time Ive seen ghttp been used for single request validation
i.e:

                             server.AppendHandlers(
					CombineHandlers(
						VerifyRequest("GET", "/v3/something/404"),
						RespondWithJSONEncoded(http.StatusNotFound, cf.CfResourceNotFound),
					),
				)

BTW: fakeCC.Count().Requests(`^/v3/some/url$\`) is a fluent wrapper I put around the server to make the test simpler

An example of using this to test some retry behaviour:
https://github.com/cloudfoundry/app-autoscaler-release/blob/bb426fb82aa9ea081984b0bcb98d24e20a4d6611/src/autoscaler/cf/retriever_test.go#L76-L93

An example of using this to stress test some failure and recovery behaviour (I deved this test to find a subtle behaviour that was plaguing one of our deployments), it is a bit complicated but I had to have a repeating behaviour on one sepcific end point an have this endpoint hit by several "threads"/users at the same time.
https://github.com/cloudfoundry/app-autoscaler-release/blob/bb426fb82aa9ea081984b0bcb98d24e20a4d6611/src/autoscaler/cf/app_test.go#L188-L248

Ive used other mocking tools were you can create a "scene" or "act" (like a play) where its easy to just write a whole bunch of request responses in sequence to elicit the incorrect behavour you are trying to fix. I think this is little easier to read in sequence than the above situations.

Maybe ghttp is the wrong tool for this but it was what was in hand without having to add more dependencies and thought it was usefull enought that the community might be able to re-use it.

@onsi
Copy link
Owner

onsi commented Sep 26, 2022

OK got it - it sounds like you are building out a more complete "fake server" vs focusing on individual end-points at a time. That's what RouteToHandler is designed to support and - as I mentioned - these two additions make sense in that context. My chief concern is around communicating to users that they are intended to be used with RouteToHandler vs AppendHandlers and this can be solved with documentation.

As for the docs - yes what you've added will appear in godoc. Gomega also has additional documentation here which acts more as a book/tutorial than the godocs. That could be a good place to explain the use case here and to guide users on when to pick which tool in the ghttp toolbox.

Lastly - scenes/acts make sense and it seems like it wouldn't be a far leap to add something like that to ghttp. If anything a scene is simply a slice of handlers or a map of routes to handlers and I could imagine rolling your own mini-DSL for that.

In any event, I'm happy to pull this in. The GitHub checks went red, though, so if you could dig into that and consider updating the docs that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants