-
Notifications
You must be signed in to change notification settings - Fork 2k
ci(imagemagick): add test isolation for functions imagemagick #3987
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
Conversation
|
This PR is failing with |
grayside
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved with a couple notes and one very optional, minor change suggestion.
| require('../index'); | ||
|
|
||
| // ImageMagick is available by default in Cloud Run Functions environments | ||
| // https://cloud.google.com/functions/1stgendocs/tutorials/imagemagick-1st-gen.md#importing_dependencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-checked that Imagemagick is still included in the system packages for v2+: https://cloud.google.com/functions/docs/reference/system-packages ✅
| // ImageMagick is available by default in Cloud Run Functions environments | ||
| // https://cloud.google.com/functions/1stgendocs/tutorials/imagemagick-1st-gen.md#importing_dependencies | ||
| // Manually install it for testing only. | ||
| execSync('sudo apt-get install imagemagick -y'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion(non-blocking): Move this into an additional before hook below and switch to an async exec for a potentially minor performance gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
installing this async was causing issues, so I'm making sure the change happens before the before using this method
.github/config/nodejs-prod.jsonc
Outdated
| "functions/billing", // Error: Request failed with status code 500 | ||
| "functions/http/uploadFile", // npm error Missing script: "test" | ||
| "functions/ocr/app", // Error: Bucket not provided. Make sure you have a "bucket" property in your request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: I do not know if you have criteria on removing the (untested) flag beyond the test fixes in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diff is unrelated to my intended changes; it should just be "move a check from skip in prod to skip in dev". I think there's some changes in main since this branch was checked out causing this
Description
Fixes #
Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.
Checklist
npm test(see Testing)npm run lint(see Style)GoogleCloudPlatform/nodejs-docs-samples. Not a fork.