test: use native fetch with mock server #702
Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
wolfy1339
added
the
Type: Maintenance
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
ready for review ;) |
|
Can you explain what this PR does? How is it improving tests? |
|
It uses tests, which use the native fetch implementation, thus avoiding that our tests are pure mocks and giving use confidence, that native fetch really works with our implementation.I could actually implement it differently and we could test e.g. node-fetch@v2 too. |
let's not do that 😅 |
test/mockRequestHttpServer.ts
Outdated
There was a problem hiding this comment.
can you stick to the dash filename convention?
test/request-common.test.ts
Outdated
| it("should error when globalThis.fetch is undefined", async () => { | ||
| expect.assertions(1); | ||
|
|
||
| const originalFetch = globalThis.fetch; | ||
| // @ts-expect-error force undefined to mimic older node version | ||
| globalThis.fetch = undefined; | ||
|
|
||
| try { | ||
| await request("GET /orgs/me"); | ||
| } catch (error) { | ||
| expect(error.message).toEqual( | ||
| "fetch is not set. Please pass a fetch implementation as new Octokit({ request: { fetch }}). Learn more at https://github.com/octokit/octokit.js/#fetch-missing", | ||
| ); | ||
| } finally { | ||
| globalThis.fetch = originalFetch; | ||
| } | ||
| }); |
There was a problem hiding this comment.
is vitest running tests in parallel? This one needs to run alone to avoid side effects
There was a problem hiding this comment.
extracted test to separate file to ensure it is run alone ;)
|
Done :) |
There was a problem hiding this comment.
Looks good to me!
@wolfy1339 do you have any more questions/concerns?
|
🎉 This issue has been resolved in version 9.1.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
needs #700