fastify / fastify-compress

Fastify compression utils
MIT License
199 stars 46 forks source link

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

Closed stanleyxu2005 closed 5 months ago

stanleyxu2005 commented 5 months ago

Checklist

stanleyxu2005 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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: https://github.com/fastify/fastify-compress/pull/291/commits/ffe60f61defc6a70e5015e9b9ecfa355d606b6b4

And fetch did: https://github.com/fastify/fastify-compress/pull/291/commits/c1882b91b6044f45c197cf8562ece0133ea5919d

stanleyxu2005 commented 5 months ago

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

gurgunday commented 5 months ago

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

mcollina commented 5 months ago

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