Feature/increase test coverage #204

Vunovati · 2021-03-09T09:43:52Z

Fix #202 Increase the test coverage for the plugin.

The coverage is:

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    98.85 |    93.27 |      100 |    98.83 |                   |
 index.js |    98.85 |    93.27 |      100 |    98.83 |       272,277,278 |
----------|----------|----------|----------|----------|-------------------|

Previously it was:

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    96.56 |    90.29 |      100 |    96.51 |                   |
 index.js |    96.56 |    90.29 |      100 |    96.51 |... 83,480,481,496 |
----------|----------|----------|----------|----------|-------------------|

To test the scenarios where temporary files are stored to disk, we needed to be able to predict what the temporary file
name would be. Previously the temp file id was generated by hexoid, but now I needed to make the id generation strategy injectable. This way, I can simulate the scenario where the temp file cannot be changed or where the temp file is missing.

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

By injecting toID as a parameter we make it possible to test errors caused by file access errors.

mcollina · 2021-03-09T10:05:03Z

CI is failing: https://github.com/fastify/fastify-multipart/pull/204/checks?check_run_id=2065221022#step:6:390

simoneb · 2021-03-09T11:18:48Z

test/multipart-disk.test.js

+
+  await fastify.listen(0)
+
+  const tempFilePath = path.join(os.tmpdir(), fileId + 0 + path.extname(filePath))


https://node-tap.org/docs/api/fixtures/

Thanks for the suggestion. I used tap's tmpdir in the next commit.

- use node-tap's testdir as a replacement for os.tmpdir - inject tmpdir in saveRequestFiles - do not inject toID in saveRequestFiles

simoneb · 2021-03-10T07:47:59Z

test/legacy/multipart.test.js

@@ -181,6 +181,88 @@ test('should error if it is not multipart', { skip: process.platform === 'win32'
  })
 })

+test('should error if handler is not a function', { skip: process.platform === 'win32' }, function (t) {


Do you know why these two tests wouldn't work on windows? Also, is there a reason why you're adding tests to this legacy folder? The name suggests these are old tests, I'm assuming new tests should be written outside of this folder perhaps.

I assume it is connected to already existing issue where tests are failing on windows for the legacy api #110.
As per readme the legacy api is not compatible with windows.

The legacy use cases also take part in the test coverage computation. I assume the folder is named legacy to group the tests for the legacy callback api. In this test we are covering the error that happens in the function handleLegacyMultipartApi.

Understood, I didn't know there was a legacy API, this explains it.

simoneb · 2021-03-10T07:49:11Z

test/multipart-disk.test.js

+  }
+})
+
+test('should not throw on request files cleanup error', { skip: process.platform === 'win32' }, async function (t) {


same question for this test. it doesn't seem to be doing anything that wouldn't work on windows

The reason for having this test skip the Windows platform is different.

I think the test is flaky on Windows. It failed on our windows CI with node 12.x but it ran successfully on a Windows 10 laptop with node 12.x. (I can't prove that, you'll have to trust me :) )
My theory is that this could be similar to the Timing Caveat on windows described in node-tap documentation where removal of the tempdir is asynchronous. tapjs/tapjs#677

In the test we are using rimraf which has a known issue that could be causing this: isaacs/rimraf#72

Makes sense, thanks for clarifying.

you can remove rimraf. recursive folder removal is now part of Node.js. See https://nodejs.org/api/fs.html#fs_fs_rmdirsync_path_options and others.

This module still supports Node 10 though

Yes, recursive is not supported in Node 10.

simoneb · 2021-03-10T08:58:13Z

The 3 remaining uncovered lines are something that can't be tested or do you think you can cover them too?

Vunovati · 2021-03-10T11:35:02Z

The 3 remaining uncovered lines are something that can't be tested or do you think you can cover them too?

I was unable to reproduce the case where those three lines would be used.

This functionality was created in #44

The unit test for the functionality was created but those three lines are not covered with that test.
What I tried was appending N streams to the FormData with either varying sizes or varying and push delays.
Still the three lines remained untouched.

I am not comfortable enough with the untested use cases to say that we can safely remove these lines. That's why I left them uncovered.

simoneb · 2021-03-10T12:06:16Z

Fair enough. You can open this PR up for review

mcollina

lgtm

Vunovati added 4 commits March 8, 2021 22:45

Remove unreachable code

6d01024

Test invalid handler and callback for legacy multipart

f448f67

Make id generation for saveRequestFiles injectable

3b1e8e8

By injecting toID as a parameter we make it possible to test errors caused by file access errors.

Use unlink instead of rm to support Node 10 and 12

634428a

simoneb reviewed Mar 9, 2021

View reviewed changes

Vunovati added 2 commits March 9, 2021 14:20

Fix test scenario message

d2a1c39

- use node-tap's testdir as a replacement for os.tmpdir - inject tmpdir in saveRequestFiles - do not inject toID in saveRequestFiles

Skip cleanup tests on Windows

d1d2a1e

simoneb reviewed Mar 10, 2021

View reviewed changes

simoneb approved these changes Mar 10, 2021

View reviewed changes

Vunovati marked this pull request as ready for review March 11, 2021 08:15

mcollina approved these changes Mar 11, 2021

View reviewed changes

mcollina merged commit af5680d into fastify:master Mar 11, 2021

This was referenced Mar 12, 2021

chore(deps-dev): bump fastify-multipart from 4.0.0 to 4.0.1 nestjs/nest#6644

Closed

chore(deps-dev): bump fastify-multipart from 4.0.0 to 4.0.1 lancygoyal/nest#474

Closed

chore(deps-dev): bump fastify-multipart from 4.0.0 to 4.0.1 geogekuo/nest#796

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/increase test coverage #204

Feature/increase test coverage #204

Vunovati commented Mar 9, 2021 •

edited

Loading

mcollina commented Mar 9, 2021

simoneb Mar 9, 2021

Vunovati Mar 9, 2021

simoneb Mar 10, 2021

Vunovati Mar 10, 2021

simoneb Mar 10, 2021

simoneb Mar 10, 2021

Vunovati Mar 10, 2021

simoneb Mar 10, 2021

mcollina Mar 10, 2021

simoneb Mar 10, 2021

Vunovati Mar 10, 2021

simoneb commented Mar 10, 2021

Vunovati commented Mar 10, 2021

simoneb commented Mar 10, 2021

mcollina left a comment


		await fastify.listen(0)

		const tempFilePath = path.join(os.tmpdir(), fileId + 0 + path.extname(filePath))

Feature/increase test coverage #204

Feature/increase test coverage #204

Conversation

Vunovati commented Mar 9, 2021 • edited Loading

Checklist

mcollina commented Mar 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simoneb commented Mar 10, 2021

Vunovati commented Mar 10, 2021

simoneb commented Mar 10, 2021

mcollina left a comment

Choose a reason for hiding this comment

Vunovati commented Mar 9, 2021 •

edited

Loading