Skip to content

Fix #6: Throw a structured exception when a command fails to start #7

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

Merged
merged 2 commits into from
Jul 11, 2021

Conversation

gzm0
Copy link
Contributor

@gzm0 gzm0 commented Jul 10, 2021

No description provided.

@gzm0 gzm0 requested a review from sjrd July 10, 2021 08:29
@gzm0 gzm0 linked an issue Jul 10, 2021 that may be closed by this pull request
@gzm0 gzm0 force-pushed the structured-failure branch from 6ae9201 to 3c17fcb Compare July 10, 2021 08:32

@Test
def failingCommand: Unit = {
val run = ExternalJSRun.start(List("false"), ExternalJSRun.Config())(_.close())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be surprised if this works on Windows.

class ExternalJSRunFailureTest {

def assertFails(future: Future[Unit])(
pf: PartialFunction[Throwable, Unit]) = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either make this private or give it an explicit result type (or both).


@Test
def abortedCommand: Unit = {
val run = ExternalJSRun.start(List("cat"), ExternalJSRun.Config()) { _ =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would also fail on Windows.

try {
builder.start()
} catch {
case t: Throwable =>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably restrict this to NonFatal exceptions, no?

@gzm0 gzm0 force-pushed the structured-failure branch from 3c17fcb to d562afd Compare July 11, 2021 10:09
@gzm0 gzm0 requested a review from sjrd July 11, 2021 10:11
@sjrd sjrd merged commit c79a923 into scala-js:master Jul 11, 2021
@gzm0 gzm0 deleted the structured-failure branch July 11, 2021 10:52
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.

Throw a structured exception when a command fails to start
2 participants