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

Polish test case for PR: replace into-stream to Readable.from #291

Merged
merged 8 commits into from
Mar 31, 2024

Conversation

stanleyxu2005
Copy link
Contributor

@stanleyxu2005 stanleyxu2005 commented Mar 30, 2024

Checklist

@stanleyxu2005
Copy link
Contributor Author

stanleyxu2005 commented Mar 30, 2024

I'd like to polish the test case @climba03003 and @mcollina create. But it's strage that your previous PR and this PR are failing at my local env.

The length of input and output strings are completly mismatched

image

@stanleyxu2005
Copy link
Contributor Author

stanleyxu2005 commented Mar 30, 2024

If I replace unidici.request with unidici.fetch, it works at my local env (but still partially failed with GitHub pipelines. Maybe the api of unidici is different for legacy Nodejs versions)

  const response1 = await fetch(`${address}/compress`)
  const response2 = await fetch(`${address}/no-compress`)
  const body1 = await response1.text()
  const body2 = await response2.text()
  t.equal(body1, body2)
  t.equal(body1.length, twoByteUnicodeContent.length)
  t.equal(body2.length, twoByteUnicodeContent.length)

Am I discovered an encoding issue for unidici? 😆

@gurgunday
Copy link
Member

That's interesting

node 14 doesn't support undici fetch I think... you might want to try node-fetch@2 if you'd like to use it here

@climba03003
Copy link
Member

I don't understand why it is strange on your environment. The test case works fine between different OS and Node versions.

I have also tested on my local environment.
What's the problem of undici.request ?

@gurgunday
Copy link
Member

I don't understand why it is strange on your environment. The test case works fine between different OS and Node versions.

I have also tested on my local environment. What's the problem of undici.request ?

@climba03003 I was going to ask if you had an idea on why undici.request might be returning a different response text length, maybe there is some post processing based on the charset of the response?

So request didn't work: ffe60f6

And fetch did: c1882b9

package.json Outdated Show resolved Hide resolved
@stanleyxu2005
Copy link
Contributor Author

stanleyxu2005 commented Mar 30, 2024

I don't understand why it is strange on your environment. The test case works fine between different OS and Node versions.

I have also tested on my local environment. What's the problem of undici.request ?

Here are my thoughts of failing or not failing @climba03003

In my attached img, the input length (334k) and output length (284k) are not identical.

image

(1) The previous PR you created, only checks the responses of compressed and uncompressed. They are identical on supported platforms (including my dev env1 - Win11), but for some reasons, not identical on my dev env2 - a legacy Windows.

(2) In my PR, I check the input and output length in additional. That's why it is failed.

(3) unidici seems to handle encoding differently, I cannot tell, if the output string is still the okay, from the length, I dont think so. After I have switched to node-fetch, the input and output length identical (which is more easier to understand). I'm not familiar with unidici, so I cannot say if unidici currput the data or not.

(4) The core implementation of your previous PR is proven functional

Co-authored-by: Gürgün Dayıoğlu <hey@gurgun.day>
Signed-off-by: Qian (Stanley) Xu <stanleyxu2005@users.noreply.github.com>
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Please use use my test. undici.fetch() correctly decode this. undici.request() for some reason does not. I'll open an issue.

Use undici@5.

@gurgunday
Copy link
Member

gurgunday commented Mar 30, 2024

@mcollina undici fetch doesn’t work on node 14 I think, do you know if that’s the case?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

You are right, there is no web streams in node v14.

@mcollina mcollina merged commit 8fe3652 into fastify:master Mar 31, 2024
19 checks passed
@stanleyxu2005 stanleyxu2005 deleted the test/refactor-regression-test branch March 31, 2024 09:43
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.

4 participants