Skip to content
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.

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

Add pretest script to download binaries before running test #128

Closed
wants to merge 1 commit into from

Conversation

mikedidomizio
Copy link

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.

@derhuerst
Copy link
Collaborator

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 added a commit that referenced this pull request Dec 21, 2023
derhuerst added a commit that referenced this pull request Dec 21, 2023
@mikedidomizio
Copy link
Author

mikedidomizio commented Dec 23, 2023

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.

@derhuerst
Copy link
Collaborator

"pretest": "npm run build"

This sounds good!

@mikedidomizio
Copy link
Author

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.

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