Use fetch-mock instead of nock #28481
Conversation
|
No failed tests 🎉 |
Codecov ReportBase: 67.19% // Head: 67.18% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #28481 +/- ##
==========================================
- Coverage 67.19% 67.18% -0.01%
==========================================
Files 3328 3328
Lines 96904 96882 -22
Branches 12293 12293
==========================================
- Hits 65113 65094 -19
+ Misses 26703 26700 -3
Partials 5088 5088
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
… 2-dbs-picker-tests
ranquild
changed the title
| }); | ||
|
|
||
| const successResponse = { status: "ok" }; | ||
|
|
||
| it("should GET", async () => { | ||
| nock(location.origin).get("/hello").reply(200, successResponse); | ||
| fetchMock.get("path:/hello", { status: 200, body: successResponse }); |
There was a problem hiding this comment.
:path matcher allows us to skip the origin, which is an improvement over nock already :)
There was a problem hiding this comment.
I like the plan! Later we can add https://www.npmjs.com/package/fetch-mock-jest
frontend/src/metabase/lib/api.js
Outdated
| _makeRequest(...args) { | ||
| if (isTest) { | ||
| return this._makeRequestWithFetch(...args); | ||
| } else { | ||
| return this._makeRequestWithXhr(...args); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think it's worth a comment; we can link this PR here
There was a problem hiding this comment.
Agree, added a comment and linked to the issue
|
One thing that might help you track down future work when we're migrating everything to fetch is, I randomly picked a few of the failed tests from this comment (for future readers you might need to look at the comment history, currently it's 19:40 GMT) From this screenshot You can see that Cypress doesn't type the first few characters (I found another test that missed the first 2 characters). IIRC, I've encountered this problem before in my test a while ago, the cause at that time was like something triggered the rerender and causes the first few keystrokes to disappear. I'm not sure what it is this time, but the symptom is very similar. |
Related to #28489
Problems with
nock:jest. There are known memory and performance issues that are not solved for a year Jest --detectLeaks reports memory leaks on nock.cleanAll() and nock.restore() nock/nock#2358, Jest leaking memory even after calling nock.restore() nock/nock#2383httpandhttpsmodule on a very low level. What we actually need is something that overwrites the globalfetchfunction with something mock-friendly.fetch-mock is a popular library for request mocking, but it works only with
fetch. And if you try to usefetchwith our cypress tests, about ~20-40 of them fail for no obvious reason, which makes me think that cypress handles xhr and fetch differently. So the solution I came up with is to usefetchin tests only for now and migrate fromnockasap because it becomes a real CI blocker.This PR converts just one single test as a proof of concept. We can migrate our existing
nock-based tests in follow-up PRs.This change is