fix: remove initial global HTTP client usage #20128
Conversation
This commit is an effort to further isolate tests by removing shared used of the global default HTTP client.
Make easy, low-risk removals of default HTTP client usage. Some application code calls CloseIdleConnections on the default HTTP client which can cause other code under test using the default HTTP client to be impacted. This should reduce test flakiness.
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
buenos-nachos
left a comment
There was a problem hiding this comment.
Everything looks good to me, but I'd rather this get a second opinion from someone with more backend context
(Also, for future reference, Kayla and I got tagged as code owners because the PR changed a file in /site, but the main intent for that check is to get more eyes on the TypeScript parts of the codebase. Kayla can absolutely hold her own when it comes to backend stuff – I'm definitely still getting my feet wet, though, so I'm going to be less useful. I'd say that if you're not updating any TypeScript code, you can go ahead and tag any backend engineer for changes like these.)
|
Nice 😎 It looks good to me. I went looking for other types of invocations that might use the default client (like Might be a good opportunity to start documenting some testing practices in the CONTRIBUTING guide (using |
This PR makes the initial steps at removing usage of the global Go HTTP client, which was seen to have impacts on test flakiness in coder/internal#1020. The first commit removes uses from tests, with the exception of one test that is tightly coupled to the default client. The second commit makes easy/low-risk removals from application code (I'm new to the project). This should have some impact to reduce flakiness.