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

Public test API to set URL params #233

Closed
achiku opened this Issue Feb 25, 2017 · 7 comments

Comments

Projects
None yet
5 participants
@achiku

achiku commented Feb 25, 2017

Thank you for creating such a wonderful library!! I have gone through issues/prs, and now put a proposal up. Any comments are welcomed!

Proposal

How do you think about adding a function to set URL params in context, which is implicitly declared as an API only for test. Testing as a public API is borrowed from this slide Advanced Testing with Go.

Background

First of all, as it's discussed in several issues, it's not good idea to make context key public or basic type since it may conflict with other keys set from different packages, and accidentally overwrite values. Secondally, it is also not good idea to make a setVars function public, since it is fundamentally not necessary for this library to work as intended, and widen its API boundary, which tend to increase difficulty to maintain a clean project.

However, it is still useful if library users can add URL params to context for testing. The main reason is that users can separate middlewares, and other routing mechanism aside when they test a function that implements http.Handler interface.

Implementation

Below is just an example.

Related issues/prs

@elithrar

This comment has been minimized.

Show comment
Hide comment
@elithrar

elithrar Feb 25, 2017

Member
Member

elithrar commented Feb 25, 2017

@achiku

This comment has been minimized.

Show comment
Hide comment
@achiku

achiku Feb 26, 2017

No difference in implementation, but clearly stating "this function is for test" by making it public test API like TestSetURLParam(r *http.Request, val map[string]string) *http.Request, the library can provide a feature making unit test finer grain without encouraging misuse of context.Context modification in production code.

achiku commented Feb 26, 2017

No difference in implementation, but clearly stating "this function is for test" by making it public test API like TestSetURLParam(r *http.Request, val map[string]string) *http.Request, the library can provide a feature making unit test finer grain without encouraging misuse of context.Context modification in production code.

@andrefsp

This comment has been minimized.

Show comment
Hide comment
@andrefsp

andrefsp Jul 5, 2017

Is there any development on this? IMO this is a quite important issue to be solved as it makes unittest a lot more complicated than it should.

andrefsp commented Jul 5, 2017

Is there any development on this? IMO this is a quite important issue to be solved as it makes unittest a lot more complicated than it should.

@grim-fendango

This comment has been minimized.

Show comment
Hide comment
@grim-fendango

grim-fendango Dec 6, 2017

Contributor

I think the provided implementation strikes a good balance between testibility and discouraging bad behaviour. Could we please get this into a PR and merged ASAP?

Contributor

grim-fendango commented Dec 6, 2017

I think the provided implementation strikes a good balance between testibility and discouraging bad behaviour. Could we please get this into a PR and merged ASAP?

@grim-fendango

This comment has been minimized.

Show comment
Hide comment
@grim-fendango

grim-fendango Dec 7, 2017

Contributor

Hey peeps, I've cherry picked the commit provided by @achiku and spun up a PR would love to hear your feedback 😸

Contributor

grim-fendango commented Dec 7, 2017

Hey peeps, I've cherry picked the commit provided by @achiku and spun up a PR would love to hear your feedback 😸

@grim-fendango

This comment has been minimized.

Show comment
Hide comment
@grim-fendango

grim-fendango Dec 9, 2017

Contributor

Hey we can close this now 🎉 since this PR has been merged.

Contributor

grim-fendango commented Dec 9, 2017

Hey we can close this now 🎉 since this PR has been merged.

@kisielk

This comment has been minimized.

Show comment
Hide comment
@kisielk

kisielk Dec 9, 2017

Member

Right on :)

Member

kisielk commented Dec 9, 2017

Right on :)

@kisielk kisielk closed this Dec 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment