Test: Enhance cross-platform test reliability and update CI workflow #1334
Conversation
There was a problem hiding this comment.
Hey, if you could add these changes to the workflow to verify that Windows really works well in the tests. https://github.com/expressjs/multer/pull/1326/files
ShubhamOulkar
force-pushed
the
fix-test-windows
branch
7 times, most recently
from
1900e29 to
b22d654
Compare
ShubhamOulkar
force-pushed
the
fix-test-windows
branch
from
b22d654 to
bdfa4e3
Compare
ShubhamOulkar
changed the title
| node-version: 'lts/*' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install --ignore-scripts --include=dev |
There was a problem hiding this comment.
npm ci would have been a better choice but we're missing a lockfile 🫣
There was a problem hiding this comment.
npm ciwould 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.
There was a problem hiding this comment.
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:
- 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. - No lockfile is less code, less code is less maintenance and fewer occasions for potential conflicts.
Creates:
- 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?
- 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.
- Your installs are significantly slower on CI as the entire dependency tree must be resolved from scratch.
- 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!"
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Sebastian Beltran <[email protected]>
test/disk-storage.js
Outdated
|
|
||
| function assertFileProperties (file, name) { | ||
| const expectedSize = util.fileSizeByName(name) | ||
| assert.strictEqual(file.fieldname, path.parse(name).name) |
There was a problem hiding this comment.
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 🤔
.github/workflows/ci.yml
Outdated
| include: | ||
| - name: Node.js 10.x | ||
| node-version: "10.24" | ||
| node-version: "10" |
There was a problem hiding this comment.
I personally think that we should test the lowest version we support, instead of the latest patch version. My reasoning is twofold:
- 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
- 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...)
There was a problem hiding this comment.
I think this will fail because of dev dependencies compatibility. I prefer latest but let's test it 🤔.
6 participants
This PR updates test assertions for cross platforms. All 61 tests were run and passed successfully on both Windows and Linux environments. Also made changes in CI workflow.
Tasks
Issue taken from #1035 (comment)