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

fix: parse statusText from the extraInfo event #7798

Merged
merged 1 commit into from Nov 26, 2021

Conversation

OrKoN
Copy link
Collaborator

@OrKoN OrKoN commented Nov 25, 2021

Issues: #7458

@OrKoN
Copy link
Collaborator Author

OrKoN commented Nov 25, 2021

@josepharhar PTAL (extracted your change from #7640)

@josepharhar
Copy link
Collaborator

Does this include the test I added with empty status text?

@OrKoN
Copy link
Collaborator Author

OrKoN commented Nov 25, 2021

Overlooked it: I am gonna add it in a moment.

@OrKoN OrKoN force-pushed the HTTPResponse-extraInfo-statusText branch from 0394fb0 to 6ad0b24 Compare November 25, 2021 20:56
@OrKoN OrKoN force-pushed the HTTPResponse-extraInfo-statusText branch from 6ad0b24 to 3823a21 Compare November 25, 2021 21:06
@OrKoN OrKoN enabled auto-merge (squash) November 26, 2021 07:03
Comment on lines +105 to +107
const firstLine = extraInfo.headersText.split('\r', 1)[0];
if (!firstLine) return;
const match = firstLine.match(/[^ ]* [^ ]* (.*)/);
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be great to have unit testы on this parsing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, the reason is that Puppeteer does not have unit tests. The tests that are called "unit" are added with this PR (one existing test and one empty status text test).

@sadym-chromium
Copy link
Collaborator

It would be really great to have the parsing unit-tested. If you have some strong reasons not to do it, please clarify them and go ahead with merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants