Fix: throwFileSizeLimit value respects per-request override

[Hackerbee]

From the comments in the file, options contains instance level configuration and opts contains per-request overrides. Hence the above conditional statement doesn't respect per-request override.

Fix: options should be replaced with opts.

Regression introduced in commit c47f60a.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Hackerbee]

How should I attach test report?

[Uzlopak]

You have to add a unit test and then we run the ci pipeline.

[Hackerbee]

I will look into the failed test case over this weekend.

[Hackerbee]

Synced the fork and rebased the branch with master.

[Hackerbee]

Regarding the failed test before (test/multipart-fileLimit.test.js) that failed only on windows, the error in the logs doesn't make much sense to me.

# Subtest: test/multipart-fileLimit.test.js
    # Subtest: should throw fileSize limitation error when consuming the stream
        1..4
        ok 1 - expect truthy value
        ok 2 - the file is not consumed yet
        ok 3 - expect truthy value
        ok 4 - should be equal
    ok 1 - should throw fileSize limitation error when consuming the stream # time=67.3ms
    
    # Subtest: should throw fileSize limitation error when consuming the stream MBs
        1..4
        ok 1 - expect truthy value
        ok 2 - the file is not consumed yet
        ok 3 - expect truthy value
        ok 4 - should be equal
    ok 2 - should throw fileSize limitation error when consuming the stream MBs # time=241.029ms
    
    # Subtest: should NOT throw fileSize limitation error when consuming the stream
        1..5
        ok 1 - expect truthy value
        ok 2 - the file is not consumed yet
        ok 3 - expect truthy value
        ok 4 - should not be equivalent
        ok 5 - should be equal
    ok 3 - should NOT throw fileSize limitation error when consuming the stream # time=30.504ms
    
    1..3
    # time=385.406ms
not ok 15 - test/multipart-fileLimit.test.js # time=385.406ms
  ---
  env: {}
  file: test/multipart-fileLimit.test.js
  timeout: 90000
  command: C:\hostedtoolcache\windows\node\20.4.0\x64\node.exe
  args:
    - test/multipart-fileLimit.test.js
  stdio:
    - 0
    - pipe
    - 2
  cwd: D:\a\fastify-multipart\fastify-multipart
  exitCode: null
  signal: SIGTERM
  ...

I don't understand why there would be a SIGTERM.
There could be 2 possible situations:

1. if it's from the test running beyond the timeout limit (90 seconds), but this particular test should not take so long.
2. something happened on the github action test runner machine which resulted in SIGTERM?

I also tried running them on laptops (macos and windows) and it is passing on both of them. Thus, I am failing to understand what could be wrong.

@Uzlopak, Would you mind running the CI again?

[Uzlopak]

But still we need a unit test to avoid regressions

[Hackerbee]

But still we need a unit test to avoid regressions

I agree, got sidelined with a peculiar observation. Let me add the tests before we go ahead with the merge. I'll need to study the existing tests to figure out how to go about writing it.

On side note,
I tested on my laptops with different node versions - 16.20.1, 18.17.0 , 20.5.0.
With 16 and 18, the tests ran without any failures on both macos and windows, but when ran with node 20, I saw timeouts on both macos and windows.
The failing test is big.test.js. Then I saw that it was skipped on CI and by setting CI=true, all tests passed.
Regardless, interesting point is that the test passed in approx 10-13 seconds when run with node 16 and node 18 but failed to finish before timeout on node 20.

[Uzlopak]

We had alot of issues with node 20 in fastify core.
And yes somehow the unit tests are flaky on windows.

This actually indicates some deeper issues. But i guess nobody felt it that painful to investigate them further.

Lets run the tests. See what we can achieve.

[mcollina]

lgtm