test: fix Fetch/HTTP2 tests

[metcoder95]

This relates to...

• HTTP2 support on Undici

Rationale

Changes

Features

Bug Fixes

• Possible flaky tests for H2

Breaking Changes and Deprecations

Status

• I have read and agreed to the Developer's Certificate of Origin
• Tested
• Benchmarked (optional)
• Documented
• Review ready
• In review
• Merge ready

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (e39a632) 85.54% compared to head (d36665a) 85.55%.
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2263   +/-   ##
=======================================
  Coverage   85.54%   85.55%           
=======================================
  Files          76       76           
  Lines        6858     6859    +1     
=======================================
+ Hits         5867     5868    +1     
  Misses        991      991           

Files Changed	Coverage Δ
index-fetch.js	100.00% <100.00%> (ø)

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

[metcoder95]

I believe we have a small bug on Node for the streams. For v16, although it is in the docs, the Readable.toWeb is missing, but not on v18 nor v20.

On v16.20:

 stream [Function: Readable] {
	ReadableState: [Function: ReadableState],
   _fromList: [Function: fromList],
   from: [Function (anonymous)],
   wrap: [Function (anonymous)]
 }

v18

stream [Function: Readable] {
   ReadableState: [Function: ReadableState],
   _fromList: [Function: fromList],
   from: [Function (anonymous)],
   fromWeb: [Function (anonymous)],
   toWeb: [Function (anonymous)],
   wrap: [Function (anonymous)]
 }

[metcoder95]

cc @mcollina @ronag @KhafraDev

[mcollina]

Can you fix the v16 problem?

[metcoder95]

Sure, do you refer to check on Node and possibly open a PR?
The only issue with the tests is that Node v16 does not exposes Redeable.toWeb although it says so in the docs

[mcollina]

CI fails on Node v16. It should not.

[metcoder95]

Sure! But the issue is that on Node.js v16, Redeable does not export the toWeb method. I validated this on the local.

From CI:

  # Subtest: [Fetch] Should handle h2 request with body (stream)
        1..8
        not ok 1 - Readable.toWeb is not a function
          ---
          stack: |
            Test.<anonymous> (test/fetch/http2.js:169:22)
          at:
            line: 169
            column: 22
            file: test/fetch/http2.js
            function: Test.<anonymous>
          type: TypeError
          tapCaught: returnedPromiseRejection
          test: "[Fetch] Should handle h2 request with body (stream)"
          source: |2
                  },
                  body: Readable.toWeb(stream),
            ---------------------^
                  duplex: 'half'
                }

On Node v16

~ ❯ node --require='stream'
Welcome to Node.js v16.20.0.
Type ".help" for more information.
> stream.Readable
[Function: Readable] {
  ReadableState: [Function: ReadableState],
  _fromList: [Function: fromList],
  from: [Function (anonymous)],
  wrap: [Function (anonymous)]
}

[metcoder95]

Should I skip the test on v16?

[KhafraDev]

Yes

[mcollina]

v16 is still failing

[metcoder95]

It failed due to redirect-request: https://github.com/nodejs/undici/actions/runs/6195437501/job/16820155843?pr=2263#step:14:10764

We have #2259 for that. I might look at that afterwards.

Retried the CI to see if solves by itself

[KhafraDev]

we could split redirect-request into multiple files to help narrow down which test is failing, we did it for the fetch abort tests and it made debugging them much easier

[metcoder95]

I'm starting to believe that the fact we were running the CI twice per pr was causing some inconsistencies while running the tests. Now everything seems green, including v20 🤔

[KhafraDev]

I hope it was that easy