Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix(httpext): return error on unsupported 101 response #1172
fix(httpext): return error on unsupported 101 response #1172
Changes from all commits
12bd929
b3d9fd5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not using
HTTPMultiBin
here to avoid a circular reference.Sidenote: I know creating test servers at runtime like this is fairly inexpensive and often encouraged in Go testing, but this feels like a needlessly heavy end-to-end test, when a simple unit test would've been enough if
MakeRequest
worked with interfaces instead of concretehttp
APIs. Tools like moq can help with generating test structs/mocks. Let me know if this can be avoided, and your thoughts on the interface approach.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed. As we will need it at some point or another
In this particular part above you are basically doing what httpmultibin is doing ... just in a smaller scale. While I have no problems in the current situation and as a quick solution for the fact httpmultibin is using netext(if I remember correctly), for something that is probably not needed - I am fine with it. And if this would be the only place where this is needed - it's fine. But if we add more tests here (more accurately move them from the
http
js module here) we would probably need to use httpmultibin either way ;).About the sidenote: My problem with mocking is that ... it tests your assumptions .. it doesn't test whether they are true. In the particular case, here it's probably fine, but in general I would say "No". Also if you go through most of the tests using httpmultibin .. if you need to set mocks for half of them it's probably going to be more work than the httpmultibin :(
I would say that definitely there are places that we can optimize and there are use cases for mocks (database connections mainly ;)) but in general if the price is not high I prefer to use the real thing ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Though typically mocking would be reserved for unit tests only. We should also have extensive integration and end-to-end tests, which we currently do. I suppose this distinction of the traditional test pyramid is not that important in Go where the end-to-end approach in this case is very cheap. Still, having a clear separation of test type would be helpful in other ways. For a quick sanity check I'd rather run the full unit test suite in seconds, than a suite with tests of all type in minutes. I can run the full suite once before the last push, or wait for CI. But again, I'm not sure how important this is in Go, which has good test caching, from what I've seen.
Anyways, we can continue this discussion at another time/place. :)
You're right about fixing the circular import. I'll refactor it if it needs to be used again here.