ffmpeg-static icon indicating copy to clipboard operation
ffmpeg-static copied to clipboard

Add pretest script to download binaries before running test

Open mikedidomizio opened this issue 2 years ago • 4 comments

This adds a pretest script to download both binaries simultaneously before running the tests.

Without doing this an error is thrown as the binary doesn't exist. It makes sense to at least have them downloaded before even attempting to run the tests.

mikedidomizio avatar Oct 20 '23 13:10 mikedidomizio

Thanks for sending a PR, I don't understand the reasoning though:

It is common practice in the npm ecosystem to run npm install before npm test. In this case, because the ffmpeg-static repo uses the workspaces feature, you'll have to run npm install --workspaces before running npm test.

derhuerst avatar Dec 21 '23 17:12 derhuerst

Hey @derhuerst, thanks for the reply.

I'm just checking back in on this, I think my code change isn't from a clean install so currently the code change isn't the correct solution.

I checked it out from a clean install and I think though there needs to be another step that might not be apparent for someone checking this out for the first time. Right now to run tests successfully you need to first build. If you run npm install --workspaces from a clean install you'll get "No workspaces found!" because no workspace package.json exist, those are created from the build-packages.js script.

To run the tests successfully requires a build first:

# creates package.json files in workspaces
npm run build
# after the following step is when the binary exists
npm i
# now the tests pass
npm test

The GitHub actions build first before testing.

I agree, it's common practice to run npm install before npm test, but it is not always a requirement to build things before test.

I'm a fan of projects having as little friction as possible to get up and running successfully, whether through better build steps or just updated documentation. I think it was just a small thing I noticed when working on this for another project. I think I'd update this PR to be a "pretest": "npm run build",, or "build:test": "npm run build && npm test", if someone wanted the ability to test directly without build.

At this point though if a change is not really desired, I'm fine closing this.

mikedidomizio avatar Dec 23 '23 00:12 mikedidomizio

"pretest": "npm run build"

This sounds good!

derhuerst avatar Dec 24 '23 12:12 derhuerst

I was wrong, "npm run build" wouldn't be enough. You have to do install after building, which means you end up with:

"pretest": "npm run build && npm install",

I don't really like having an install in there because like you said, you'd be running install anyways, but I've committed it. I'm still fine to close this.

mikedidomizio avatar Dec 28 '23 22:12 mikedidomizio