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

Doesn't work within jest #318

Closed
timsuchanek opened this issue Aug 13, 2020 · 5 comments · Fixed by #319
Closed

Doesn't work within jest #318

timsuchanek opened this issue Aug 13, 2020 · 5 comments · Fixed by #319

Comments

@timsuchanek
Copy link
Contributor

When running an undici client in jest, jest is doing some kind of override magic in the background, that makes this check for a buffer in undici fail.

Reproduction: https://github.com/timsuchanek/undici-buffer-repro

I'm already working on a fix :) I suggest adding a || Buffer.isBuffer(body) in https://github.com/mcollina/undici/blob/master/lib/client.js#L946

@ronag
Copy link
Member

ronag commented Aug 13, 2020

I'm very curious what happens here. It shouldn't be possible for anyone to modify the Request object. Are they overriding Buffer.from or something?

@mcollina
Copy link
Member

I would highly recommend to not use jest as it provides different globals for each test, making all those check fail.
Anyway, a fix would be welcomed. Please also add a test for it.

@timsuchanek
Copy link
Contributor Author

@mcollina I guess you recommend using tap instead? We have all our tests written in jest, but it has its limits.

We once hit a similar problem in Prisma Client, where we had an instanceof Date check. As a naive library author I thought that's sufficient - but there is some test environment, I think it was https://quokkajs.com/ that overrides the global Date, therefore the instanceof doesn't work anymore.
So we had to move to a Object.prototype.toString test instead.

timsuchanek added a commit to timsuchanek/undici that referenced this issue Aug 13, 2020
@timsuchanek
Copy link
Contributor Author

It's kinda awkward to add jest in there, but this is the only sane way I found to add the test #319

@SimenB
Copy link
Member

SimenB commented Aug 15, 2020

Sorry about Jest causing issues 🙁 The issue to track (if anybody wants) in Jest for this is jestjs/jest#2549. I've tried multiple times to fix it without luck. I believe it can be solved if these two are ever solved:

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 a pull request may close this issue.

4 participants