-
-
Notifications
You must be signed in to change notification settings - Fork 15
fix: Check for kUnknown ExecutableFormat #84
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
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.
Nice catch, thanks!
(Posting the link to where you found this - #12 (comment) for context for others)
if any, testing is wanted
Yea, would be nice if we could add a test to test/cli.mjs
that tries to create a temporary file with an invalid format (I guess it could even be an empty file) and checks that postject throws the right exception
Co-authored-by: Darshan Sen <raisinten@gmail.com>
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.
lgtm
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.
I ran CI on my fork - https://app.circleci.com/pipelines/github/RaisinTen/postject/3/workflows/a0657615-c702-4934-88b5-2b3bb41706ac and there are some relevant errors. We can fix the linting errors by running npm run format
. This test also seems to be running out of time on CI, so we could add a timeout to fix that.
Co-authored-by: Darshan Sen <raisinten@gmail.com>
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.
Unsure what, if any, testing is wanted for PRs. Let me know. :)