Skip to content

Add integration test for R errors #547

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.

Sign up for GitHub

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 5 commits into from
Oct 3, 2024
Merged

Add integration test for R errors #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
753fffd
Clearer control flow in `TryFrom<WireMessage> for Message`
lionel- Sep 25, 2024
fca167d
Return `Status` instead of `ExecuteReply`
lionel- Sep 26, 2024
1330af8
Add test for errors
lionel- Sep 26, 2024
c67d0cd
Document situation with error replies
lionel- Sep 26, 2024
978026e
Assert `status` field and return `execution_count`
lionel- Sep 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions crates/amalthea/src/fixtures/dummy_frontend.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ use crate::connection_file::ConnectionFile;
use crate::session::Session;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I wish every reply was structured like this

pub enum ExecuteReply {
    Success(ExecuteReplySuccess),
    Exception(ExecuteReplyException),
}

There would be a lot of duplication, but I think it would make the structure a lot cleaner and less confusing. And ExecuteReplyException's special case of adding the execution_count would fit right in with no issues in this world - the ExecuteReplyException would just have 1 more field than most exception structs

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a bit like how we have ExecuteResponse, but "response" is a confusing name because its really a "reply" that correctly encapsulates the two variants in the way id like them all to

pub enum ExecuteResponse {
    Reply(ExecuteReply),
    ReplyException(ExecuteReplyException),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my feeling too.

use crate::socket::socket::Socket;
use crate::wire::execute_input::ExecuteInput;
use crate::wire::execute_reply::ExecuteReply;
use crate::wire::execute_request::ExecuteRequest;
use crate::wire::jupyter_message::JupyterMessage;
use crate::wire::jupyter_message::Message;
use crate::wire::jupyter_message::ProtocolMessage;
use crate::wire::jupyter_message::Status;
use crate::wire::status::ExecutionState;
use crate::wire::wire_message::WireMessage;

Expand Down Expand Up @@ -160,12 +160,25 @@ impl DummyFrontend {
Message::read_from_socket(&self.shell_socket).unwrap()
}

/// Receive from Shell and assert ExecuteReply message
pub fn recv_shell_execute_reply(&self) -> ExecuteReply {
/// Receive from Shell and assert `ExecuteReply` message.
/// Returns `execution_count`.
pub fn recv_shell_execute_reply(&self) -> u32 {
let msg = self.recv_shell();

assert_match!(msg, Message::ExecuteReply(data) => {
data.content
assert_eq!(data.content.status, Status::Ok);
data.content.execution_count
})
}

/// Receive from Shell and assert `ExecuteReplyException` message.
/// Returns `execution_count`.
pub fn recv_shell_execute_reply_exception(&self) -> u32 {
let msg = self.recv_shell();

assert_match!(msg, Message::ExecuteReplyException(data) => {
assert_eq!(data.content.status, Status::Error);
data.content.execution_count
})
}

Expand Down
2 changes: 2 additions & 0 deletions crates/amalthea/src/socket/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,8 @@ impl Shell {
let r = req.send_reply(reply, &self.socket);
r
},
// FIXME: Ark already created an `ExecuteReplyException` so we use
// `.send_reply()` instead of `.send_error()`. Can we streamline this?
Err(err) => req.send_reply(err, &self.socket),
}
}
Expand Down
12 changes: 9 additions & 3 deletions crates/amalthea/src/wire/error_reply.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,11 @@ use crate::wire::jupyter_message::MessageType;
use crate::wire::jupyter_message::Status;

/// Represents an error that occurred after processing a request on a
/// ROUTER/DEALER socket
/// ROUTER/DEALER socket.
///
/// This is the payload of a response to a request. Note that, as an exception,
/// responses to `"execute_request"` include an `execution_count` field. We
/// represent these with an `ExecuteReplyException`.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct ErrorReply {
/// The status; always Error
Expand All @@ -25,9 +29,11 @@ pub struct ErrorReply {
}

/// Note that the message type of an error reply is generally adjusted to match
/// its request type (e.g. foo_request => foo_reply)
/// its request type (e.g. foo_request => foo_reply). The message type
/// implemented here is only a placeholder and should not appear in any
/// serialized/deserialized message.
impl MessageType for ErrorReply {
fn message_type() -> String {
String::from("error")
String::from("*error payload*")
}
}
5 changes: 4 additions & 1 deletion crates/amalthea/src/wire/execute_error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,10 @@ use serde::Serialize;
use crate::wire::exception::Exception;
use crate::wire::jupyter_message::MessageType;

/// Represents an exception that occurred while executing code
/// Represents an exception that occurred while executing code.
/// This is sent to IOPub. Not to be confused with `ExecuteReplyException`
/// which is a special case of a message of type `"execute_reply"` sent to Shell
/// in response to an `"execute_request"`.
#[derive(Debug, Serialize, Deserialize, Clone)]
pub struct ExecuteError {
/// The exception that occurred during execution
Expand Down
85 changes: 60 additions & 25 deletions crates/amalthea/src/wire/jupyter_message.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,57 +168,92 @@ impl TryFrom<&WireMessage> for Message {
/// messages that are received from the frontend.
fn try_from(msg: &WireMessage) -> Result<Self, Error> {
let kind = msg.header.msg_type.clone();

if kind == KernelInfoRequest::message_type() {
return Ok(Message::KernelInfoRequest(JupyterMessage::try_from(msg)?));
} else if kind == KernelInfoReply::message_type() {
}
if kind == KernelInfoReply::message_type() {
return Ok(Message::KernelInfoReply(JupyterMessage::try_from(msg)?));
} else if kind == IsCompleteRequest::message_type() {
}
if kind == IsCompleteRequest::message_type() {
return Ok(Message::IsCompleteRequest(JupyterMessage::try_from(msg)?));
} else if kind == IsCompleteReply::message_type() {
}
if kind == IsCompleteReply::message_type() {
return Ok(Message::IsCompleteReply(JupyterMessage::try_from(msg)?));
} else if kind == InspectRequest::message_type() {
}
if kind == InspectRequest::message_type() {
return Ok(Message::InspectRequest(JupyterMessage::try_from(msg)?));
} else if kind == InspectReply::message_type() {
}
if kind == InspectReply::message_type() {
return Ok(Message::InspectReply(JupyterMessage::try_from(msg)?));
} else if kind == ExecuteRequest::message_type() {
}
if kind == ExecuteReplyException::message_type() {
if let Ok(data) = JupyterMessage::try_from(msg) {
return Ok(Message::ExecuteReplyException(data));
}
// else fallthrough to try `ExecuteRequest` which has the same message type
}
if kind == ExecuteRequest::message_type() {
return Ok(Message::ExecuteRequest(JupyterMessage::try_from(msg)?));
} else if kind == ExecuteReply::message_type() {
}
if kind == ExecuteReply::message_type() {
return Ok(Message::ExecuteReply(JupyterMessage::try_from(msg)?));
} else if kind == ExecuteResult::message_type() {
}
if kind == ExecuteResult::message_type() {
return Ok(Message::ExecuteResult(JupyterMessage::try_from(msg)?));
} else if kind == ExecuteInput::message_type() {
}
if kind == ExecuteError::message_type() {
return Ok(Message::ExecuteError(JupyterMessage::try_from(msg)?));
}
if kind == ExecuteInput::message_type() {
return Ok(Message::ExecuteInput(JupyterMessage::try_from(msg)?));
} else if kind == CompleteRequest::message_type() {
}
if kind == CompleteRequest::message_type() {
return Ok(Message::CompleteRequest(JupyterMessage::try_from(msg)?));
} else if kind == CompleteReply::message_type() {
}
if kind == CompleteReply::message_type() {
return Ok(Message::CompleteReply(JupyterMessage::try_from(msg)?));
} else if kind == ShutdownRequest::message_type() {
}
if kind == ShutdownRequest::message_type() {
return Ok(Message::ShutdownRequest(JupyterMessage::try_from(msg)?));
} else if kind == KernelStatus::message_type() {
}
if kind == KernelStatus::message_type() {
return Ok(Message::Status(JupyterMessage::try_from(msg)?));
} else if kind == CommInfoRequest::message_type() {
}
if kind == CommInfoRequest::message_type() {
return Ok(Message::CommInfoRequest(JupyterMessage::try_from(msg)?));
} else if kind == CommInfoReply::message_type() {
}
if kind == CommInfoReply::message_type() {
return Ok(Message::CommInfoReply(JupyterMessage::try_from(msg)?));
} else if kind == CommOpen::message_type() {
}
if kind == CommOpen::message_type() {
return Ok(Message::CommOpen(JupyterMessage::try_from(msg)?));
} else if kind == CommWireMsg::message_type() {
}
if kind == CommWireMsg::message_type() {
return Ok(Message::CommMsg(JupyterMessage::try_from(msg)?));
} else if kind == CommClose::message_type() {
}
if kind == CommClose::message_type() {
return Ok(Message::CommClose(JupyterMessage::try_from(msg)?));
} else if kind == InterruptRequest::message_type() {
}
if kind == InterruptRequest::message_type() {
return Ok(Message::InterruptRequest(JupyterMessage::try_from(msg)?));
} else if kind == InterruptReply::message_type() {
}
if kind == InterruptReply::message_type() {
return Ok(Message::InterruptReply(JupyterMessage::try_from(msg)?));
} else if kind == InputReply::message_type() {
}
if kind == InputReply::message_type() {
return Ok(Message::InputReply(JupyterMessage::try_from(msg)?));
} else if kind == InputRequest::message_type() {
}
if kind == InputRequest::message_type() {
return Ok(Message::InputRequest(JupyterMessage::try_from(msg)?));
} else if kind == StreamOutput::message_type() {
}
if kind == StreamOutput::message_type() {
return Ok(Message::StreamOutput(JupyterMessage::try_from(msg)?));
} else if kind == UiFrontendRequest::message_type() {
}
if kind == UiFrontendRequest::message_type() {
return Ok(Message::CommRequest(JupyterMessage::try_from(msg)?));
} else if kind == JsonRpcReply::message_type() {
}
if kind == JsonRpcReply::message_type() {
return Ok(Message::CommReply(JupyterMessage::try_from(msg)?));
}
return Err(Error::UnknownMessageType(kind));
Expand Down
26 changes: 22 additions & 4 deletions crates/ark/tests/kernel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use amalthea::wire::jupyter_message::Message;
use amalthea::wire::jupyter_message::Status;
use amalthea::wire::kernel_info_request::KernelInfoRequest;
use ark::fixtures::DummyArkFrontend;
use stdext::assert_match;
Expand Down Expand Up @@ -28,11 +27,30 @@ fn test_execute_request() {
frontend.send_execute_request("42");
frontend.recv_iopub_busy();

assert_eq!(frontend.recv_iopub_execute_input().code, "42");
let input = frontend.recv_iopub_execute_input();
assert_eq!(input.code, "42");
assert_eq!(frontend.recv_iopub_execute_result(), "[1] 42");

frontend.recv_iopub_idle();

let reply = frontend.recv_shell_execute_reply();
assert_eq!(reply.status, Status::Ok);
assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count)
}

#[test]
fn test_execute_request_error() {
let frontend = DummyArkFrontend::lock();

frontend.send_execute_request("stop('foobar')");
frontend.recv_iopub_busy();

let input = frontend.recv_iopub_execute_input();
assert_eq!(input.code, "stop('foobar')");
assert!(frontend.recv_iopub_execute_error().contains("foobar"));

frontend.recv_iopub_idle();

assert_eq!(
frontend.recv_shell_execute_reply_exception(),
input.execution_count
);
}
Loading