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

Properly surface non-serializable errors in tests #212

Merged
merged 3 commits into from
Mar 21, 2025

Conversation

sangaline
Copy link
Contributor

AVA runs tests in separate processes, and it uses IPC to communicate them back to the main process for logging and reporting. If an error isn't serializable, then the IPC fails and we end up not being able to see the actual error or even what test failed which is as delightful as it sounds. The main culprit of this has been Axios when there's a Nock error.

This PR improves the situation by adding an Axios response interceptor and sanitizing any errors so that they're serializable. The heavy lifting on the sanitization is done by the serialize-error package which I've added as a dev dependency. It cleans up issues like functions, buffers, and circular references which are the common culprits of non-serializability.

If you want to test this yourself, you can remove fixtures with rm test/fixtures/sdk.test.ts.json and then run npm run test to run the tests. You'll get the opaque error on main and the real error on this branch.

Copy link
Contributor

@katiemckeon katiemckeon left a comment

Choose a reason for hiding this comment

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

Adding a dev dependency is pretty lightweight and 100% justified here. New code in test/utils/makeAxiosErrorsSerializable.ts is well documented the code makes sense.

Looks good to me.

@sangaline sangaline merged commit a0571ad into main Mar 21, 2025
6 checks passed
@sangaline sangaline deleted the ews-handle-unserializable-axios-errors branch March 21, 2025 00:56
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