Extend TestAPI interface to allow for custom context.Context #469
Conversation
|
Warning Rate limit exceeded@coury-clark has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent changes introduce context handling in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- humatest/humatest.go (9 hunks)
Additional comments not posted (8)
humatest/humatest.go (8)
7-7: Added import forcontextpackage.This is necessary for the new context handling features.
49-49: Ensure backward compatibility with the newDoCtxmethod and its wrapperDo.The addition of
DoCtxalongside the existingDomethod, which now wrapsDoCtxusingcontext.Background(), maintains backward compatibility while extending functionality.Also applies to: 51-51
64-64: Ensure backward compatibility with the newGetCtxmethod and its wrapperGet.The
GetCtxmethod is correctly implemented with a context parameter, and theGetmethod is updated to wrap this new method, ensuring backward compatibility.Also applies to: 66-66
79-79: Ensure backward compatibility with the newPostCtxmethod and its wrapperPost.Similar to
GetCtx, thePostCtxmethod introduces context handling, and thePostmethod wraps it appropriately.Also applies to: 81-81
94-94: Ensure backward compatibility with the newPutCtxmethod and its wrapperPut.The
PutCtxmethod adds context support, and thePutmethod wraps it, maintaining the existing API's functionality.Also applies to: 96-96
109-109: Ensure backward compatibility with the newPatchCtxmethod and its wrapperPatch.The
PatchCtxmethod is a new addition that handles context, and thePatchmethod is updated to use this new method, ensuring no disruption to existing functionality.Also applies to: 111-111
124-124: Ensure backward compatibility with the newDeleteCtxmethod and its wrapperDelete.The
DeleteCtxmethod correctly implements the new context handling feature, and theDeletemethod wraps it, preserving existing behavior.Also applies to: 126-126
141-141: Review implementation of context handling inDoCtxand its usage in other methods.The
DoCtxmethod is well-implemented with comprehensive handling of different argument types and proper context integration. This method is then correctly used as the underlying implementation for other context-specific methods (GetCtx,PostCtx,PutCtx,PatchCtx,DeleteCtx), ensuring consistency and reducing code duplication.Also applies to: 164-164, 195-195, 198-198, 205-205, 208-208, 215-215, 218-218, 225-225, 228-228, 235-235, 238-238
humatest/humatest.go
Outdated
| // api.Put("/foo", "X-My-Header: my-value", MyBody{Foo: "bar"}) | ||
| PutCtx(ctx context.Context, path string, args ...any) *httptest.ResponseRecorder | ||
|
|
||
| // Put wraps [TestAPI.PutCtx] using [context.Background]. |
There was a problem hiding this comment.
For this one and others I think you should address these:
- the comment should be taken from existing code, here your changes lead to have less information in the comment, so in the godoc
- you should add something to existing comment to mention PutCtx should be preferred when there is an existing context
There was a problem hiding this comment.
Ah, I didn't realize comments inside interfaces don't support links in pkgsite (it does in most editors, for example). I've copied the comment over and edited them to reflect the correct signature.
There was a problem hiding this comment.
I think you missed or ignored my second comment.
The fact you could use in each old code there is a version with a context that should be preferred
There was a problem hiding this comment.
Ah, sorry it seemed I missed or ignored this. Let me clarify!
The comments do make some case for the scenarios you'd use it, perhaps implicitly
Get performs a GET request against the API using [context.Background] in the
// [http.Request]
While it's true this is an implicit instruction, I'd expect anyone familiar with Go to understand when they do or do not need to provide their own context.
To address preference, I don't necessarily think from the perspective of the library there is a preference - this is simply a utility after all. Similarly, http has no preference if users use NewRequest or NewRequestWithContext, it's just an option. I'd expect anyone using this library to be able to make their own choices about whether or not they need to provide a context. That said, I didn't update the website documentation (which in practice, is where I get most of the examples from), so please look at da24dac.
ccoVeille
left a comment
There was a problem hiding this comment.
One small remark, nothing really important, so I'm OK 👍
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- docs/docs/tutorial/writing-tests.md (1 hunks)
Additional context used
LanguageTool
docs/docs/tutorial/writing-tests.md
[uncategorized] ~163-~163: A comma might be missing here. (AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
Context: ...ttps://pkg.go.dev/context#Context), for example you may need to test an authenticated r...
[uncategorized] ~163-~163: When ‘request-specific’ is used as a modifier, it is usually spelled with a hyphen. (SPECIFIC_HYPHEN)
Context: ...ticated route, or test using some other request specific values. ```go func TestGetGreeting(t *...
Markdownlint
docs/docs/tutorial/writing-tests.md
17-17: Column: 1 (MD010, no-hard-tabs)
Hard tabs
18-18: Column: 1 (MD010, no-hard-tabs)
Hard tabs
19-19: Column: 1 (MD010, no-hard-tabs)
Hard tabs
21-21: Column: 1 (MD010, no-hard-tabs)
Hard tabs
22-22: Column: 1 (MD010, no-hard-tabs)
Hard tabs
23-23: Column: 1 (MD010, no-hard-tabs)
Hard tabs
24-24: Column: 1 (MD010, no-hard-tabs)
Hard tabs
26-26: Column: 1 (MD010, no-hard-tabs)
Hard tabs
31-31: Column: 1 (MD010, no-hard-tabs)
Hard tabs
36-36: Column: 1 (MD010, no-hard-tabs)
Hard tabs
37-37: Column: 1 (MD010, no-hard-tabs)
Hard tabs
38-38: Column: 1 (MD010, no-hard-tabs)
Hard tabs
43-43: Column: 1 (MD010, no-hard-tabs)
Hard tabs
44-44: Column: 1 (MD010, no-hard-tabs)
Hard tabs
45-45: Column: 1 (MD010, no-hard-tabs)
Hard tabs
46-46: Column: 1 (MD010, no-hard-tabs)
Hard tabs
47-47: Column: 1 (MD010, no-hard-tabs)
Hard tabs
51-51: Column: 1 (MD010, no-hard-tabs)
Hard tabs
52-52: Column: 1 (MD010, no-hard-tabs)
Hard tabs
53-53: Column: 1 (MD010, no-hard-tabs)
Hard tabs
54-54: Column: 1 (MD010, no-hard-tabs)
Hard tabs
55-55: Column: 1 (MD010, no-hard-tabs)
Hard tabs
56-56: Column: 1 (MD010, no-hard-tabs)
Hard tabs
57-57: Column: 1 (MD010, no-hard-tabs)
Hard tabs
58-58: Column: 1 (MD010, no-hard-tabs)
Hard tabs
59-59: Column: 1 (MD010, no-hard-tabs)
Hard tabs
60-60: Column: 1 (MD010, no-hard-tabs)
Hard tabs
61-61: Column: 1 (MD010, no-hard-tabs)
Hard tabs
62-62: Column: 1 (MD010, no-hard-tabs)
Hard tabs
63-63: Column: 1 (MD010, no-hard-tabs)
Hard tabs
64-64: Column: 1 (MD010, no-hard-tabs)
Hard tabs
65-65: Column: 1 (MD010, no-hard-tabs)
Hard tabs
67-67: Column: 1 (MD010, no-hard-tabs)
Hard tabs
68-68: Column: 1 (MD010, no-hard-tabs)
Hard tabs
69-69: Column: 1 (MD010, no-hard-tabs)
Hard tabs
70-70: Column: 1 (MD010, no-hard-tabs)
Hard tabs
71-71: Column: 1 (MD010, no-hard-tabs)
Hard tabs
72-72: Column: 1 (MD010, no-hard-tabs)
Hard tabs
73-73: Column: 1 (MD010, no-hard-tabs)
Hard tabs
74-74: Column: 1 (MD010, no-hard-tabs)
Hard tabs
75-75: Column: 1 (MD010, no-hard-tabs)
Hard tabs
76-76: Column: 1 (MD010, no-hard-tabs)
Hard tabs
77-77: Column: 1 (MD010, no-hard-tabs)
Hard tabs
78-78: Column: 1 (MD010, no-hard-tabs)
Hard tabs
82-82: Column: 1 (MD010, no-hard-tabs)
Hard tabs
83-83: Column: 1 (MD010, no-hard-tabs)
Hard tabs
84-84: Column: 1 (MD010, no-hard-tabs)
Hard tabs
85-85: Column: 1 (MD010, no-hard-tabs)
Hard tabs
86-86: Column: 1 (MD010, no-hard-tabs)
Hard tabs
88-88: Column: 1 (MD010, no-hard-tabs)
Hard tabs
90-90: Column: 1 (MD010, no-hard-tabs)
Hard tabs
91-91: Column: 1 (MD010, no-hard-tabs)
Hard tabs
92-92: Column: 1 (MD010, no-hard-tabs)
Hard tabs
93-93: Column: 1 (MD010, no-hard-tabs)
Hard tabs
94-94: Column: 1 (MD010, no-hard-tabs)
Hard tabs
95-95: Column: 1 (MD010, no-hard-tabs)
Hard tabs
97-97: Column: 1 (MD010, no-hard-tabs)
Hard tabs
98-98: Column: 1 (MD010, no-hard-tabs)
Hard tabs
110-110: Column: 1 (MD010, no-hard-tabs)
Hard tabs
111-111: Column: 1 (MD010, no-hard-tabs)
Hard tabs
113-113: Column: 1 (MD010, no-hard-tabs)
Hard tabs
117-117: Column: 1 (MD010, no-hard-tabs)
Hard tabs
119-119: Column: 1 (MD010, no-hard-tabs)
Hard tabs
121-121: Column: 1 (MD010, no-hard-tabs)
Hard tabs
122-122: Column: 1 (MD010, no-hard-tabs)
Hard tabs
123-123: Column: 1 (MD010, no-hard-tabs)
Hard tabs
124-124: Column: 1 (MD010, no-hard-tabs)
Hard tabs
128-128: Column: 1 (MD010, no-hard-tabs)
Hard tabs
130-130: Column: 1 (MD010, no-hard-tabs)
Hard tabs
132-132: Column: 1 (MD010, no-hard-tabs)
Hard tabs
133-133: Column: 1 (MD010, no-hard-tabs)
Hard tabs
134-134: Column: 1 (MD010, no-hard-tabs)
Hard tabs
135-135: Column: 1 (MD010, no-hard-tabs)
Hard tabs
137-137: Column: 1 (MD010, no-hard-tabs)
Hard tabs
138-138: Column: 1 (MD010, no-hard-tabs)
Hard tabs
139-139: Column: 1 (MD010, no-hard-tabs)
Hard tabs
143-143: Column: 1 (MD010, no-hard-tabs)
Hard tabs
145-145: Column: 1 (MD010, no-hard-tabs)
Hard tabs
147-147: Column: 1 (MD010, no-hard-tabs)
Hard tabs
148-148: Column: 1 (MD010, no-hard-tabs)
Hard tabs
149-149: Column: 1 (MD010, no-hard-tabs)
Hard tabs
151-151: Column: 1 (MD010, no-hard-tabs)
Hard tabs
152-152: Column: 1 (MD010, no-hard-tabs)
Hard tabs
153-153: Column: 1 (MD010, no-hard-tabs)
Hard tabs
167-167: Column: 1 (MD010, no-hard-tabs)
Hard tabs
169-169: Column: 1 (MD010, no-hard-tabs)
Hard tabs
170-170: Column: 1 (MD010, no-hard-tabs)
Hard tabs
171-171: Column: 1 (MD010, no-hard-tabs)
Hard tabs
173-173: Column: 1 (MD010, no-hard-tabs)
Hard tabs
174-174: Column: 1 (MD010, no-hard-tabs)
Hard tabs
175-175: Column: 1 (MD010, no-hard-tabs)
Hard tabs
176-176: Column: 1 (MD010, no-hard-tabs)
Hard tabs
180-180: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
160-160: null (MD014, commands-show-output)
Dollar signs used before commands without showing output
|
LGTM 👍 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #469 +/- ##
==========================================
- Coverage 92.84% 92.82% -0.03%
==========================================
Files 22 22
Lines 3761 3778 +17
==========================================
+ Hits 3492 3507 +15
- Misses 225 227 +2
Partials 44 44 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
@coury-clark this looks great, thank you! 👍
FYI you can already do something similar by making your own http.Request and then using api.Adapter().ServeHTTP(w, r) but this is much simpler!
This PR extends the humatest.TestAPI interface to allow for a custom context.Context to be passed into the request. This is useful for applications where the context is used to store / propagate request specific values, such as authentication / authorization information. This is essentially porting a wrapper I had to write internally back into the mainline, it seems universally useful enough to me - let me know if you disagree!
This does introduce a breaking change in the TestAPI interface, which is difficult to avoid given the
humatest.Newfunction returns the interface. If that isn't acceptable, please let me know!Alternative naming options are similar to
httppackage - ie. GetWithContext. Let me know if you'd prefer that style.Thanks for huma!
Summary by CodeRabbit
New Features
Ctxmethods forDo,Get,Post,Put,Patch, andDeleteto accept acontext.Contextparameter.Documentation
context.Contextin Go tests for scenarios like testing authenticated routes.