Skip to content

Test: Enhance cross-platform test reliability and update CI workflow #1334

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.

Sign up for GitHub

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Test: Enhance cross-platform test reliability and update CI workflow #1334

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
66a16f2
fix(tests): make file size assertions cross-platform
ShubhamOulkar Jun 6, 2025
bdfa4e3
ci: run test for windows and linux
ShubhamOulkar Jun 7, 2025
f45a1fe
Update ci.yml
ShubhamOulkar Aug 11, 2025
031edfb
remove path ignore
ShubhamOulkar Aug 12, 2025
e7ccbef
add test where fieldname and originalname did not match
ShubhamOulkar Aug 13, 2025
7bb1afb
test the lowest version support
ShubhamOulkar Aug 13, 2025
c22c220
fix ci
ShubhamOulkar Aug 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 75 additions & 70 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,21 +1,38 @@
name: ci

on:
- pull_request
- push
push:
branches:
- main
paths-ignore:
- '*.md'
pull_request:
paths-ignore:
- '*.md'

permissions:
contents: read

jobs:
test:
permissions:
checks: write # for coverallsapp/github-action to create new checks
contents: read # for actions/checkout to fetch code
lint:
name: Lint
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: 'lts/*'

- name: Install dependencies
run: npm install --ignore-scripts --include=dev
Copy link
Contributor

@wojtekmaj wojtekmaj Aug 11, 2025

Choose a reason for hiding this comment

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

npm ci would have been a better choice but we're missing a lockfile 🫣

Copy link
Member Author

@ShubhamOulkar ShubhamOulkar Aug 11, 2025

Choose a reason for hiding this comment

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

right ♻️

Copy link
Member

@LinusU LinusU Aug 12, 2025

Choose a reason for hiding this comment

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

npm ci would have been a better choice but we're missing a lockfile 🫣

Is it good to check in a lock file in a library though? That lockfile won't be respected when publishing to npm, so we wouldn't be testing with the same dependencies as used by our users, but instead whatever dependencies happened to be used when we generate the lockfile.

Copy link
Contributor

@wojtekmaj wojtekmaj Aug 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

One could argue that adding a lockfile is a double-edged sword, but personally I think not having a lockfile in place creates far more problems than it solves.

Solves:

  1. You will have your code tested against the latest (matching) version of declared dependencies.
    Counterargument: one can set up dependabot or renovate to update dependencies automatically or at least semi-automatically.
  2. No lockfile is less code, less code is less maintenance and fewer occasions for potential conflicts.

Creates:

  1. Let's say one of your dependencies release a new (matching) version. Unfortunately it introduced a change that causes your tests to go red. Suddenly:
    • Your tests on main branch are failing - even though they were passing yesterday and you didn't make any changes, or
    • Contributors are raising PRs that unexpectedly go red, causing confusion: did I break anything?
  2. Let's say one of your dependencies gets hijacked and releases a new version which is a malware that steals your env variables, like npm token. By not having a lockfile you reduce your "grace period" to ZERO. Your repository will be infected virtually immediately, and if stealing the token is successful, YOUR package will quickly become malware as well.
  3. Your installs are significantly slower on CI as the entire dependency tree must be resolved from scratch.
  4. People technically working on the same branch may have different lockfiles locally, causing your code to be non-deterministic: a common root cause of a classic "weird, it works on my machine!"

Copy link
Member

@LinusU LinusU Aug 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Counterargument: one can set up dependabot or renovate to update dependencies automatically or at least semi-automatically.

This is certainly possible, but then there needs to be very clear guidelines on how this is supposed to work. I've stopped contributing to a few other packages where dependabot/renovate/similar was added, because it just caused a lot of spam without any clear point of action.

Personally, I'm not sure if I can keep notifications enabled for Multer if there are too many notifications from this, as it will drown out any issue were my attention is actually needed.

Let's say one of your dependencies release a new (matching) version. Unfortunately it introduced a change that causes your tests to go red.

I do agree that this is undesirable. In practice though, I think that during all my time in the npm ecosystem I've seen this once or twice.

Let's say one of your dependencies gets hijacked and releases a new version which is a malware that steals your env variables, like npm token. By not having a lockfile you reduce your "grace period" to ZERO. Your repository will be infected virtually immediately, and if stealing the token is successful, YOUR package will quickly become malware as well.

This is an interesting point. Multer doesn't have any env variables at all setup, so this wouldn't be a problem at the moment. Then again, as you say, it would only reduce the grace period.

I think that the only way to properly prevent this is that if/when we add automatic publishing to npm, the workflow that publishes a new version should be the only one with access to that env variable, and it shouldn't run npm install at all, just npm publish.

Your installs are significantly slower on CI as the entire dependency tree must be resolved from scratch.

We currently don't have any cache step in the ci at all. Are you saying that running npm ci on a complete clean machine is faster than running npm install on a completely clean machine? I guess it is, although I'm not sure what the difference would be 🤔

Currently, the npm install step just takes ~10 seconds in our CI.

People technically working on the same branch may have different lockfiles locally, causing your code to be non-deterministic: a common root cause of a classic "weird, it works on my machine!"

True, but this will be the case when end users install Multer in both cases. So I could argue that it's better that developers find that out whilst working on the code, rather than it looks to be working for the developers, and then it's broken for all new end users.

(btw, they wouldn't have different lock files as we have package-lock=false in our .npmrc, but they would have different dependency tree; which again, would be the same as all end users)

Writing all this though, I actually came up with an argument for keeping a lock file. In the same fashion that I think that we should test with the lowest supported version of Node.js, it would probably be best to test with the lowest supported version of all dependencies as well 🤔

Although in practice, I personally feel as a lockfile will just cause conflicts in peoples branches, without providing any tangible benefits...

Anyhow, just my two cents, happy to be convinced otherwise...

I think that we should probably follow what the main Express.js repo does (or any official policy if there is one). It seems like they do not use a lock file.

Copy link
Member Author

@ShubhamOulkar ShubhamOulkar Aug 13, 2025

Choose a reason for hiding this comment

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

I think that we should probably follow what the main Express.js repo does (or any official policy if there is one). It seems like they do not use a lock file.

That's, what I did. This repo following main repo so no problems right now.


- name: Run lint
run: npm run lint
test:
strategy:
fail-fast: false
matrix:
os: [ubuntu-latest, windows-latest]
name:
- Node.js 10.x
- Node.js 11.x
Expand All @@ -35,43 +52,43 @@ jobs:

include:
- name: Node.js 10.x
node-version: "10.24"
node-version: "10"
Copy link
Member

@LinusU LinusU Aug 12, 2025

Choose a reason for hiding this comment

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

I personally think that we should test the lowest version we support, instead of the latest patch version. My reasoning is twofold:

  1. I think that we are more likely to accidentally break compatibility with an older version of Node.js, than a newer version of Node.js is to break compatibility for us
  2. Making sure that a newer version of Node.js doesn't break us is already handled by nodejs/citgm

In addition to that, even if a newer version of Node.js broke us, it would be annoying if that suddenly turned all CI red, since it would then block unrelated changes until that breakage is dealt with.

So in practice, what I'm suggesting is that we test on 10.16.0 (our declared minimum version in package.json), and then 11.0.0, 12.0.0, etc. (personally, I usually skip the odd releases as well...)

Copy link
Member Author

@ShubhamOulkar ShubhamOulkar Aug 13, 2025

Choose a reason for hiding this comment

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

I think this will fail because of dev dependencies compatibility. I prefer latest but let's test it 🤔.

npm-i: [email protected]

- name: Node.js 11.x
node-version: "11.15"
node-version: "11"
npm-i: [email protected]

- name: Node.js 12.x
node-version: "12.22"
node-version: "12"
npm-i: [email protected]

- name: Node.js 13.x
node-version: "13.14"
node-version: "13"
npm-i: [email protected]

- name: Node.js 14.x
node-version: "14.21"
node-version: "14"
npm-i: [email protected]

- name: Node.js 15.x
node-version: "15.14"
node-version: "15"
npm-i: [email protected]

- name: Node.js 16.x
node-version: "16.20"
node-version: "16"

- name: Node.js 17.x
node-version: "17.9"
node-version: "17"

- name: Node.js 18.x
node-version: "18.18"
node-version: "18"

- name: Node.js 19.x
node-version: "19.9"
node-version: "19"

- name: Node.js 20.x
node-version: "20.9"
node-version: "20"

- name: Node.js 21.x
node-version: "21"
Expand All @@ -85,26 +102,16 @@ jobs:
- name: Node.js 24.x
node-version: "24"

runs-on: ${{ matrix.os }}
steps:
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

- name: Install Node.js ${{ matrix.node-version }}
shell: bash -eo pipefail -l {0}
run: |
nvm install --default ${{ matrix.node-version }}
if [[ "${{ matrix.node-version }}" == 0.* && "$(cut -d. -f2 <<< "${{ matrix.node-version }}")" -lt 10 ]]; then
nvm install --alias=npm 0.10
nvm use ${{ matrix.node-version }}
if [[ "$(npm -v)" == 1.1.* ]]; then
nvm exec npm npm install -g [email protected]
ln -fs "$(which npm)" "$(dirname "$(nvm which npm)")/npm"
else
sed -i '1s;^.*$;'"$(printf '#!%q' "$(nvm which npm)")"';' "$(readlink -f "$(which npm)")"
fi
npm config set strict-ssl false
fi
dirname "$(nvm which ${{ matrix.node-version }})" >> "$GITHUB_PATH"
- uses: actions/checkout@v4
with:
persist-credentials: false

- name: Setup Node.js ${{ matrix.node-version }}
uses: actions/setup-node@v4
with:
node-version: ${{ matrix.node-version }}
- name: Remove npm module(s) ${{ matrix.npm-rm }}
run: npm rm --silent --save-dev ${{ matrix.npm-rm }}
if: matrix.npm-rm != ''
Expand All @@ -116,43 +123,41 @@ jobs:
- name: Install Node.js dependencies
run: npm install

- name: List environment
id: list_env
shell: bash
run: |
echo "node@$(node -v)"
echo "npm@$(npm -v)"
npm -s ls ||:
(npm -s ls --depth=0 ||:) | awk -F'[ @]' 'NR>1 && $2 { print $2 "=" $3 }' >> "$GITHUB_OUTPUT"

- name: Lint code
run: npm run lint

- name: Run tests
shell: bash
run: |
if npm -ps ls nyc | grep -q nyc; then
npm run test-ci
else
npm test
fi
run: npm run test-ci

- name: Collect code coverage
uses: coverallsapp/github-action@09b709cf6a16e30b0808ba050c7a6e8a5ef13f8d # master
if: steps.list_env.outputs.nyc != ''
- name: Upload code coverage
uses: actions/upload-artifact@v4
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
flag-name: run-${{ matrix.test_number }}
parallel: true
name: coverage-node-${{ matrix.node-version }}-${{ matrix.os }}
path: ./coverage/lcov.info
retention-days: 1

coverage:
permissions:
checks: write # for coverallsapp/github-action to create new checks
needs: test
runs-on: ubuntu-latest
steps:
- name: Upload code coverage
uses: coverallsapp/github-action@09b709cf6a16e30b0808ba050c7a6e8a5ef13f8d # master
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
parallel-finished: true
needs: test
runs-on: ubuntu-latest
permissions:
contents: read
checks: write
steps:
- uses: actions/checkout@v4

- name: Install lcov
shell: bash
run: sudo apt-get -y install lcov

- name: Collect coverage reports
uses: actions/download-artifact@v4
with:
path: ./coverage
pattern: coverage-node-*

- name: Merge coverage reports
shell: bash
run: find ./coverage -name lcov.info -exec printf '-a %q\n' {} \; | xargs lcov -o ./lcov.info

- name: Upload coverage report
uses: coverallsapp/github-action@v2
with:
file: ./lcov.info
14 changes: 13 additions & 1 deletion test/_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,26 @@ var fs = require('fs')
var path = require('path')
var stream = require('stream')

exports.filePath = function filePath (name) {
return path.join(__dirname, 'files', name)
}

exports.file = function file (name) {
return fs.createReadStream(path.join(__dirname, 'files', name))
return fs.createReadStream(this.filePath(name))
}

exports.fileSize = function fileSize (path) {
return fs.statSync(path).size
}

exports.fileSizeByName = function fileSizeByName (name) {
return this.fileSize(this.filePath(name))
}

exports.readDir = function readDir (dirPath) {
return fs.readdirSync(dirPath)
}

exports.submitForm = function submitForm (multer, form, cb) {
form.getLength(function (err, length) {
if (err) return cb(err)
Expand Down
63 changes: 18 additions & 45 deletions test/disk-storage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,21 @@
var assert = require('assert')
var deepEqual = require('deep-equal')

var fs = require('fs')
var path = require('path')
var util = require('./_util')
var multer = require('../')
var temp = require('fs-temp')
var rimraf = require('rimraf')
var FormData = require('form-data')

function assertFileProperties (file, name) {
const expectedSize = util.fileSizeByName(name)
assert.strictEqual(file.fieldname, path.parse(name).name)
Copy link
Member

@LinusU LinusU Aug 12, 2025

Choose a reason for hiding this comment

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

We should probably have tests where the fieldname and name of the file doesn't match. Otherwise this wouldn't catch if we in the library somehow mix them together 🤔

Copy link
Member Author

@ShubhamOulkar ShubhamOulkar Aug 13, 2025

Choose a reason for hiding this comment

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

Thank you @LinusU, test added in e7ccbef

assert.strictEqual(file.originalname, name)
assert.strictEqual(file.size, expectedSize)
assert.strictEqual(util.fileSize(file.path), expectedSize)
}

describe('Disk Storage', function () {
var uploadDir, upload

Expand Down Expand Up @@ -40,10 +47,7 @@ describe('Disk Storage', function () {

assert.strictEqual(req.body.name, 'Multer')

assert.strictEqual(req.file.fieldname, 'small0')
assert.strictEqual(req.file.originalname, 'small0.dat')
assert.strictEqual(req.file.size, 1778)
assert.strictEqual(util.fileSize(req.file.path), 1778)
assertFileProperties(req.file, 'small0.dat')

done()
})
Expand Down Expand Up @@ -75,10 +79,7 @@ describe('Disk Storage', function () {
assert(deepEqual(req.body.checkboxhalfempty, ['cb1', '']))
assert(deepEqual(req.body.checkboxempty, ['', '']))

assert.strictEqual(req.file.fieldname, 'empty')
assert.strictEqual(req.file.originalname, 'empty.dat')
assert.strictEqual(req.file.size, 0)
assert.strictEqual(util.fileSize(req.file.path), 0)
assertFileProperties(req.file, 'empty.dat')

done()
})
Expand Down Expand Up @@ -109,40 +110,13 @@ describe('Disk Storage', function () {

assert(deepEqual(req.body, {}))

assert.strictEqual(req.files.empty[0].fieldname, 'empty')
assert.strictEqual(req.files.empty[0].originalname, 'empty.dat')
assert.strictEqual(req.files.empty[0].size, 0)
assert.strictEqual(util.fileSize(req.files.empty[0].path), 0)

assert.strictEqual(req.files.tiny0[0].fieldname, 'tiny0')
assert.strictEqual(req.files.tiny0[0].originalname, 'tiny0.dat')
assert.strictEqual(req.files.tiny0[0].size, 122)
assert.strictEqual(util.fileSize(req.files.tiny0[0].path), 122)

assert.strictEqual(req.files.tiny1[0].fieldname, 'tiny1')
assert.strictEqual(req.files.tiny1[0].originalname, 'tiny1.dat')
assert.strictEqual(req.files.tiny1[0].size, 7)
assert.strictEqual(util.fileSize(req.files.tiny1[0].path), 7)

assert.strictEqual(req.files.small0[0].fieldname, 'small0')
assert.strictEqual(req.files.small0[0].originalname, 'small0.dat')
assert.strictEqual(req.files.small0[0].size, 1778)
assert.strictEqual(util.fileSize(req.files.small0[0].path), 1778)

assert.strictEqual(req.files.small1[0].fieldname, 'small1')
assert.strictEqual(req.files.small1[0].originalname, 'small1.dat')
assert.strictEqual(req.files.small1[0].size, 315)
assert.strictEqual(util.fileSize(req.files.small1[0].path), 315)

assert.strictEqual(req.files.medium[0].fieldname, 'medium')
assert.strictEqual(req.files.medium[0].originalname, 'medium.dat')
assert.strictEqual(req.files.medium[0].size, 13196)
assert.strictEqual(util.fileSize(req.files.medium[0].path), 13196)

assert.strictEqual(req.files.large[0].fieldname, 'large')
assert.strictEqual(req.files.large[0].originalname, 'large.jpg')
assert.strictEqual(req.files.large[0].size, 2413677)
assert.strictEqual(util.fileSize(req.files.large[0].path), 2413677)
assertFileProperties(req.files.empty[0], 'empty.dat')
assertFileProperties(req.files.tiny0[0], 'tiny0.dat')
assertFileProperties(req.files.tiny1[0], 'tiny1.dat')
assertFileProperties(req.files.small0[0], 'small0.dat')
assertFileProperties(req.files.small1[0], 'small1.dat')
assertFileProperties(req.files.medium[0], 'medium.dat')
assertFileProperties(req.files.large[0], 'large.jpg')

done()
})
Expand All @@ -160,8 +134,7 @@ describe('Disk Storage', function () {
assert.strictEqual(err.field, 'small0')
assert(deepEqual(err.storageErrors, []))

var files = fs.readdirSync(uploadDir)
assert(deepEqual(files, []))
assert(deepEqual(util.readDir(uploadDir), []))

done()
})
Expand Down
8 changes: 5 additions & 3 deletions test/express-integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ describe('Express Integration', function () {

it('should work when receiving error from fileFilter', function (done) {
function fileFilter (req, file, cb) {
cb(new Error('TEST'))
if (file.mimetype !== 'image/png') {
return cb(new Error('Only PNG files are allowed'))
}
}

var upload = multer({ fileFilter: fileFilter })
Expand All @@ -80,15 +82,15 @@ describe('Express Integration', function () {
var routeCalled = 0
var errorCalled = 0

form.append('avatar', util.file('large.jpg'))
form.append('avatar', util.file('small0.dat'))

router.post('/profile', upload.single('avatar'), function (req, res, next) {
routeCalled++
res.status(200).end('SUCCESS')
})

router.use(function (err, req, res, next) {
assert.strictEqual(err.message, 'TEST')
assert.strictEqual(err.message, 'Only PNG files are allowed')

errorCalled++
res.status(500).end('ERROR')
Expand Down
Loading
Loading