chore: remove mocha and chai

[Uzlopak]

replaces chai and mocha with native test runner of node.

found a bug in Fetch Request.

[KhafraDev]

What is the issue in fetch? Why is the webidl stuff needed?

[Uzlopak]

@KhafraDev

webidl.converters.BodyInit is not defined in request.js but we are using it in this line

undici/lib/fetch/request.js

Line 920 in a0812d8

webidl.converters.BodyInit

The unit test 'should throw error with GET/HEAD requests with body' was before just checking if a error is thrown. When i changed it to also check the message, it showed 'converter not defined'(or something similar). This was because BodyInit was not defined.
Now it throws the correct error.

[KhafraDev]

I'm assuming you imported lib/fetch/request.js and not index.js?

[Uzlopak]

@KhafraDev Yes you are right. If I import it from index.js it will work as expected. But would this not mean, that we have the risk of breaking something if we import directly from fetch/request.js and not from index.js?

[Uzlopak]

@KhafraDev

Done, but feels error prone as the hoisting order is important.

[KhafraDev]

It's either that or circular imports... I opted for this way. After all, you can't have Request by itself.

[Uzlopak]

How about adding a simple

if (webidl.converters.BodyInit === undefined) {
throw new Error('should have required correctly')
}

Or in webidl.nullableConverter it should check if the coverter is a function, and throw accordingly.

This should result in no runtime overhead but improves the chance of finding a wrong require/import

[KhafraDev]

Meh, realistically it shouldn't happen and tests should just require index.js. I don't see it being useful.

[Uzlopak]

@KhafraDev

But overall you like the changes?

[KhafraDev]

yeah, I just have no time to do a full review on it

[Uzlopak]

@KhafraDev
No worries. I just want to make sure, that there is no big reason for rejection or something i can fix now as long as it is still "warm" in my head.

[codecov-commenter]

Codecov Report

Attention: 223 lines in your changes are missing coverage. Please review.

Comparison is base (e39a632) 85.54% compared to head (65eeda3) 85.27%.
Report is 271 commits behind head on main.

Files	Patch %	Lines
lib/fetch/util.js	60.41%	57 Missing ⚠️
lib/fetch/index.js	68.78%	54 Missing ⚠️
lib/handler/RetryHandler.js	74.35%	30 Missing ⚠️
lib/cache/cache.js	12.12%	29 Missing ⚠️
lib/fetch/dataURL.js	79.31%	12 Missing ⚠️
lib/api/readable.js	83.92%	9 Missing ⚠️
lib/core/diagnostics.js	84.74%	9 Missing ⚠️
lib/eventsource/eventsource.js	96.09%	5 Missing ⚠️
lib/core/request.js	94.36%	4 Missing ⚠️
lib/client.js	95.83%	3 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2696      +/-   ##
==========================================
- Coverage   85.54%   85.27%   -0.28%     
==========================================
  Files          76       84       +8     
  Lines        6858     7587     +729     
==========================================
+ Hits         5867     6470     +603     
- Misses        991     1117     +126     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[metcoder95]

LGTM

[mcollina]

lgtm

[styfle]

Can you explain why this seems odd?

I am seeing the following:

TypeError: fetch failed
    at node:internal/deps/undici/undici:12502:13
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  [cause]: Error: unknown scheme

Perhaps this test should check for unknown scheme instead?

[Uzlopak]

Yeah, I think this is what made it odd to me.

[KhafraDev]

fetch should always reject with "fetch failed" and the cause should be the error. Basically, it's working properly.

[styfle]

I see. In that case we should remove the comment?

[Uzlopak]

I think are lacking the test that error.cause.message should be "unknown scheme".