Skip to content
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

Merged

Conversation

OlaviSau
Copy link
Contributor

I made mockNetwork even more flexible in order to accommodate more tests. Doing another intermediate PR for easier review.

@OlaviSau OlaviSau requested a review from capricorn86 as a code owner January 23, 2025 07:04
@OlaviSau OlaviSau force-pushed the feature-1502-fetch-test-improvements branch from 5153033 to da50ea0 Compare January 23, 2025 17:41
Copy link
Owner

@capricorn86 capricorn86 left a 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);
Copy link
Owner

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.

Copy link
Contributor Author

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];
Copy link
Owner

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, not really.

});
return {
Copy link
Owner

@capricorn86 capricorn86 Jan 23, 2025

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[]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converted to object :)

Copy link
Owner

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@OlaviSau OlaviSau force-pushed the feature-1502-fetch-test-improvements branch 2 times, most recently from aa241a1 to 80afe5e Compare January 24, 2025 18:45
@OlaviSau OlaviSau force-pushed the feature-1502-fetch-test-improvements branch from 80afe5e to 12a6ba4 Compare January 24, 2025 18:50
@OlaviSau OlaviSau requested a review from capricorn86 January 24, 2025 18:53
Copy link
Owner

@capricorn86 capricorn86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good ⭐

@capricorn86 capricorn86 merged commit b03fba8 into capricorn86:master Jan 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants