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 response error handling and slightly improve the digest auth code #1102
Fix response error handling and slightly improve the digest auth code #1102
Changes from 5 commits
8c27b6b
ec493b9
911c60c
0b8e412
6c85afd
c668d7a
411be0c
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 think this should be done in the
debug*
methods and I would really like a test for this, as it will be very bad if we break .. the http when you are debugging it for example ;)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.
Not sure I understand... Can you elaborate?
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 don't want to waste any time in writing of tests that would be obsolete by the time we properly fix the
http-debug
functionality, since it's currently a pile of 💩 . Much like the digest authentication, I haven't changed anything significant from before, I've only slightly rearranged and moved the pile of 💩 to a separate place so it handles all cases and is easier to properly fix in the future. But I don't want to test something whose mode of working we plan to significantly change in the near future...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 am/was left with the impression you intend on setting the state in the struct .. while I think it is okay to always get it from the request's context.
And pointing out that a bug in this code will result in the k6/http code breaking when you try to debug it which makes it important to be tested with at least a couple of tests ;) For which being able to change the log will be useful ;)
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.
Being a pile of shit is the exact reason why tests are good idea ... if it wasn't it would've had less reason for it.
Also I don't think the change will make the tests obsolete if anything the tests will show and document the change when it happens and will mostly do with (probably) small change in what we output and some caching ... both of which will not be significant problems , unlike if this code breaks or is broken in a way that nobody tested for.
Lastly "near" and "soon" are relative terms. If you say that this will be fixed in vX.Y.Z in two weeks .. I might be fine .. but given how it usually goes I much prefer if we have tests for things that we intend to change "soon" as "soon" is usually in few months ..
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 respectfully disagree with both points. I see no point if getting the state from the requests' context every time, since we already have it and we can just inject in the struct - much simpler, no contexts and no type asserts required.
But that point is completely moot, since I have no intentions whatsoever of dealing with any of that in this pull request, given the fact that we agreed to leave the proper fixes of the
http-debug
mess for another time... I won't dump whole requetss the logger, because I have no idea what issues that will bring - we'll deal with that properly when we fix the http-debug properly. This is just a preliminary step so it tracks every request, so that we don't make things even worse...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.
FFS, now codecov complains 🤦♂️
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 is as far as I'm willing to invest in testing HttpDebug for now: 411be0c