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: surface 'cause' for undici network errors #642

Merged
merged 2 commits into from
Oct 6, 2023

Conversation

nikhilgupta345
Copy link
Contributor

Resolves #641


Before the change?

Network errors from undici are presented with "fetch failed" without showing the underlying cause that undici also presents.

After the change?

Network errors now correctly propagate the 'cause' from undici

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

  • Yes
  • No

@wolfy1339 wolfy1339 added the Type: Bug Something isn't working as documented label Oct 6, 2023
@wolfy1339 wolfy1339 changed the title Surface 'cause' for undici network errors fix: surface 'cause' for undici network errors Oct 6, 2023
@gr2m
Copy link
Contributor

gr2m commented Oct 6, 2023

great PR, thank you!!! It looks like coverage dropped, could you please have a look?

------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
------------------------|---------|----------|---------|---------|-------------------
All files               |     100 |    97.82 |     100 |     100 |                   
 fetch-wrapper.ts       |     100 |    97.67 |     100 |     100 | 144               
 get-buffer-response.ts |     100 |      100 |     100 |     100 |                   
 index.ts               |     100 |      100 |     100 |     100 |                   
 version.ts             |     100 |      100 |     100 |     100 |                   
 with-defaults.ts       |     100 |      100 |     100 |     100 |                   
------------------------|---------|----------|---------|---------|-------------------
Jest: "global" coverage threshold for branches (100%) not met: 97.82%

@nikhilgupta345
Copy link
Contributor Author

nikhilgupta345 commented Oct 6, 2023

great PR, thank you!!! It looks like coverage dropped, could you please have a look?

------------------------|---------|----------|---------|---------|-------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
------------------------|---------|----------|---------|---------|-------------------
All files               |     100 |    97.82 |     100 |     100 |                   
 fetch-wrapper.ts       |     100 |    97.67 |     100 |     100 | 144               
 get-buffer-response.ts |     100 |      100 |     100 |     100 |                   
 index.ts               |     100 |      100 |     100 |     100 |                   
 version.ts             |     100 |      100 |     100 |     100 |                   
 with-defaults.ts       |     100 |      100 |     100 |     100 |                   
------------------------|---------|----------|---------|---------|-------------------
Jest: "global" coverage threshold for branches (100%) not met: 97.82%

Unfortunately I'm not sure what's going on because the test wouldn't pass unless line 144 was being executed. I also confirmed by putting console.logs immediately before and after line 144 and ensured they were both executed during the test suite. Any ideas?

image image

@gr2m
Copy link
Contributor

gr2m commented Oct 6, 2023

The line is covered but not all branches (possible execution paths) are. I think it's ok to ignore the line from coverage. I'm out on mobile right now, I forgot the syntax for jest coverage

@nikhilgupta345
Copy link
Contributor Author

The line is covered but not all branches (possible execution paths) are. I think it's ok to ignore the line from coverage. I'm out on mobile right now, I forgot the syntax for jest coverage

Aha just figured that out at the same time. Just pushed up a fix. Thanks!

@gr2m gr2m merged commit 7c9abfb into octokit:main Oct 6, 2023
7 checks passed
@github-actions
Copy link

github-actions bot commented Oct 6, 2023

🎉 This PR is included in version 8.1.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Type: Bug Something isn't working as documented
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG]: Network errors from unidici are not properly surfaced
3 participants