-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
Feature 1502 fetch test improvements #1702
Feature 1502 fetch test improvements #1702
Conversation
…e-1502-fetch-test-improvements # Conflicts: # packages/happy-dom/test/fetch/Fetch.test.ts
…viSau/happy-dom into feature-1502-fetch-test-improvements
5153033
to
da50ea0
Compare
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.
It looks good in general, just some minor suggestions for improvement 🙂 Let me know what you think!
if (event === 'response') { | ||
callback(response); | ||
const response = <HTTP.IncomingMessage>Stream.Readable.from(generate()); | ||
Object.assign(response, { headers: {}, rawHeaders: [] }, extra); |
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.
It may be useful if it defaults to 200 OK as this is the most common response.
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.
Added.
mockModule(schema, { | ||
request: (url: string, options: HTTP.RequestOptions) => { | ||
const request = { url, options }; | ||
requestHistory = [...requestHistory, request]; |
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.
Any reason for not using requestHistory.push()
here?
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.
Hmm, not really.
}); | ||
return { |
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.
Perhaps beforeResponse
can be sent in as a parameter to the mockNetwork()
function.
I that case it might be more convenient to use an object for the parameters:
function mockNetwork(options: { schema: 'http' | 'https'; responseText?: string: string[]; statusCode?: number; statusMessage?: string; headers?: { [k: string]: string }; rawHeaders?: string[]; beforeResponse: IBeforeResponseHook }): IRequestHistoryEntry[]
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.
Converted to object :)
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.
Nice :)
I think we will need to be able to send in statusCode, statusMessage and headers as well
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.
Done.
aa241a1
to
80afe5e
Compare
80afe5e
to
12a6ba4
Compare
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.
Looks good ⭐
I made mockNetwork even more flexible in order to accommodate more tests. Doing another intermediate PR for easier review.