-
Notifications
You must be signed in to change notification settings - Fork 55
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
Rework forge command invocation (broken EPIPE issue) #642
base: development
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development #642 +/- ##
===============================================
- Coverage 54.52% 54.32% -0.21%
===============================================
Files 226 227 +1
Lines 5199 5230 +31
Branches 800 809 +9
===============================================
+ Hits 2835 2841 +6
- Misses 2127 2152 +25
Partials 237 237 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 think we need to clear the timeout on process exit.
I am tempted to say we should rework the timeout Error, so that when it is caught by the try/catch in onDocumentFormatting
we report a more specific sentry status for timeout: { status: "internal_error", result: null }
. This would let us separate out timeouts from anything else.
As a sidepoint - the codecov here is really painful to review. If it is not pulling its weight we should consider turning it off. |
@kanej addressed all comments. About codecov, I'd be happy to remove it. We can tell on PR reviews what needs and what doesn't need coverage. For execWithInput, I think it's not viable to unit test. It would involve a lot of stubbing and mocking, and the added value would be almost nonexistent. For a non-stub testing case, we'd have to see what's possible in terms of process manipulation from our test case, and it can be tricky to have it be multi platform. |
Co-authored-by: John Kane <[email protected]>
Co-authored-by: John Kane <[email protected]>
The type narrowing wasn't working on the version of typescript we build with. I added a typeguard to circumvent the build problem.
|
||
return [textEdit]; | ||
function isKilledDefinedExecException(error: unknown): error is ExecException { |
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.
My previous suggestion caused a build error. So this type guard, while cumbersome does the job. We should port the error helpers from Hardhat!
This PR changes the handling of the child process spawned when running
forge
, currently only used for formatting. The broken EPIPE error happens when a process tries to write to the stdin of a dead process.