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.

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

merged 5 commits into from
Oct 3, 2024

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Sep 26, 2024

Branched from #542

I was confused for a while trying to make tests for execution errors because of some puzzling ambiguities:

  • In the Jupyter protocol all replies have nominally one type (foo_reply with foo corresponding to the request name, e.g. foo_request), but they each have two different structural types. When the status field is "error", all fields are omitted and instead the exception fields (ename, evalue, stacktrace, represented by the Rust type Exception) are included. The contents of the error variants of these messages are represented by the Rust type ErrorReply, which currently has "error" as message_type().

  • As special case, the error variant of "execute_reply" messages must also preserve the execution_count field. This is represented by the Rust type ExecuteReplyException.

    Ark handles this downstream and for this reason (I think) we don't use send_error() in Amalthea's Shell socket, we use send_reply() in both cases, which is also confusing:

    Err(err) => req.send_reply(err, &self.socket),

  • Execution errors are also signaled on IOPub with messages of type "error". These are represented by the Rust type ExecuteError.

Things changed in this PR:

  • I was confused by ErrorReply having the same message_type() (

    String::from("error")
    ) as IOPub's ExecuteError messages (
    String::from("error")
    ). AFAICT this message type is never used. The payload is included as is by error_reply(), see
    pub fn error_reply<R: ProtocolMessage>(
    . It only needs a message_type() method for type reasons involving expected traits. So I set the message type to "*error payload*" to better reflect it's only a placeholder.

  • We now take precautions in the try_from() method that deserialises Jupyter messages to disambiguate between the regular and error variants of "execute_reply".

  • The error situation is now better documented so it's not as confusing the next time we have to deal with Jupyter errors.

@lionel- lionel- force-pushed the feature/client-tests branch from 0196e97 to 1f5753d Compare September 27, 2024 09:11
Base automatically changed from feature/client-tests to main September 27, 2024 09:23
@lionel- lionel- force-pushed the feature/error-tests branch 2 times, most recently from 5dd5e53 to c74a84e Compare September 27, 2024 13:08
@lionel- lionel- force-pushed the feature/error-tests branch 2 times, most recently from 749f940 to 978026e Compare October 2, 2024 09:59
@@ -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.

@lionel- lionel- merged commit 76ddd9c into main Oct 3, 2024
6 checks passed
@lionel- lionel- deleted the feature/error-tests branch October 3, 2024 14:22
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants