Skip to content

Add coverage-focused tests for disk storage and middleware internals - #1437

Open
ashish3011 wants to merge 1 commit into
expressjs:mainfrom
ashish3011:test/coverage-100-percent
Open

Add coverage-focused tests for disk storage and middleware internals#1437
ashish3011 wants to merge 1 commit into
expressjs:mainfrom
ashish3011:test/coverage-100-percent

Conversation

@ashish3011

Copy link
Copy Markdown
  • Added targeted test cases for the disk storage flow and multipart middleware internals to exercise previously uncovered branches.
  • This improves confidence in the upload lifecycle around destination handling, error propagation, cleanup behavior, and field-name validation.
  • The goal is to raise coverage for the affected internal modules and reduce the chance of regressions in core upload behavior.

Those two files were touched as a small implementation cleanup while I was adding the coverage tests.

lib/file-appender.js: I changed the placeholder-removal logic from the old bitwise-style check to a plain [idx >= 0] check. It is functionally equivalent, but it is clearer and easier to reason about.
lib/make-middleware.js: I changed the pending-file cleanup from [indexOf] + [splice] to a [filter]-based removal. That makes the removal step more robust when multiple completion paths are happening and avoids mutating the array in a less obvious way.

@MohammedAlkindi MohammedAlkindi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ran this on Windows 11 / Node v24.18.0, since multer's CI is ubuntu-latest only and these tests touch disk storage and path handling.

Baseline, upstream/main:

$ npm test
79 passing (3s)
1 failing

This branch:

$ npm test
100 passing (3s)
1 failing

21 additional passing tests and no new failures on Windows. The one failure is identical on both and is not yours:

1) Functionality
     should rename the destination directory to a different directory:
   AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
   assert.ok(req.files[0].path.indexOf('/testforme-') >= 0)

test/functionality.js hardcodes a forward slash as the path separator, so it can never match a Windows path regardless of what the storage layer does. Pre-existing, unrelated to this PR, and worth its own issue.

One caveat about my environment so you can weight this correctly: the fixture files in my working tree are LF, because I had normalised them while working on #1450. On a fresh Windows clone of this branch they arrive CRLF and several size assertions fail before your new tests get a chance to say anything — that is the separate problem #1450 fixes, not something this PR introduces. I mention it only because if you or a reviewer try to reproduce these numbers on a clean Windows clone before #1450 lands, you will see extra failures that have nothing to do with your changes.

Not checked: whether the new assertions hold on macOS, and I did not review the lib/file-appender.js and lib/make-middleware.js edits for behavioural intent — this is a platform report on the test run, not a design review.

@ashish3011

Copy link
Copy Markdown
Author

@MohammedAlkindi Thank you so much for taking the time to pull this down and run it on Windows! This is incredibly helpful since the CI doesn't cover it.

It's great to hear that the 21 new tests are passing smoothly on your end. I appreciate the heads-up on the pre-existing / path separator issue in test/functionality.js and the context regarding #1450. I'll leave my PR as-is since the failure is unrelated to my changes.

Thanks again for the thorough platform report!

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.

2 participants