Skip to content
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

test: serialize and deserialize RPC error messages #8411

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Aug 17, 2023

We demonstrate a bug with how error messages are serialized. Currently there are two "Error:" prefixes after serializing and deserializing. This is because the prefix hasn't been stripped in the initial serialization. We should strip the prefix since the severity information is recorded elsewhere in the diagnostic data.

@Alizter Alizter requested a review from rgrinberg August 17, 2023 01:02
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 17, 2023

@rgrinberg I think the correct fix for this would be to make user_message.ml remember prefixes in the type. That way we can avoid including them when serialising. Otherwise, I don't see an easy way of removing Error: short of regexing.

@Alizter Alizter changed the title test: serialize and reserialize RPC error messages test: serialize and deserialize RPC error messages Aug 17, 2023
@rgrinberg rgrinberg requested a review from pmwhite August 17, 2023 10:39
@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from 63281ae to 8f9d38f Compare August 17, 2023 14:18
@Alizter Alizter mentioned this pull request Aug 18, 2023
12 tasks
@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from 8f9d38f to 2b46350 Compare August 19, 2023 00:29
@Alizter Alizter requested a review from rgrinberg August 19, 2023 00:30
@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch 2 times, most recently from 692876c to bac10c3 Compare August 23, 2023 11:37
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 23, 2023

@rgrinberg I've pushed another test which shows how location printing is kind of funny. I need some help scrubbing the output however.

@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from bac10c3 to b55b2e5 Compare August 23, 2023 14:41
@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from b55b2e5 to fd58a5d Compare August 23, 2023 21:00
@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from fd58a5d to 4c93ceb Compare August 24, 2023 12:35
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 24, 2023

@rgrinberg I've also included dyn serialisation so we can more finely compare the Pp structure that has been serialised.

@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from 4c93ceb to 77ce937 Compare August 24, 2023 17:47
@Alizter
Copy link
Collaborator Author

Alizter commented Aug 24, 2023

@rgrinberg I've moved the tests now and added yet another example of an error message that changes. All these examples will be fixed by my upcoming PR.

@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch 3 times, most recently from a74d86f to 8c09cc7 Compare August 24, 2023 18:35
We demonstrate a bug with how error messages are serialized. Currently
there are two "Error:" prefixes after serializing and deserializing.
This is because the prefix hasn't been stripped in the initial
serialization. We should strip the prefix since the severity information
is recorded elsewhere in the diagnostic data.

Signed-off-by: Ali Caglayan <alizter@gmail.com>
@Alizter Alizter force-pushed the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch from 8c09cc7 to 3c924a3 Compare August 24, 2023 20:24
@rgrinberg rgrinberg merged commit a4d6daa into ocaml:main Aug 26, 2023
@Alizter Alizter deleted the ps/branch/test__serialize_and_reserialize_rpc_error_messages branch August 26, 2023 16:10
@Alizter Alizter mentioned this pull request Aug 26, 2023
2 tasks
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.

2 participants