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

feat: handler use deserialization version of the message with test_serialization=true #86 #87

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

jaugustin
Copy link
Contributor

Hello

This fix issue #86 it allow to completely test serialization/deserialization flow in messageHandler

@jaugustin jaugustin force-pushed the fix-serialize-flow branch from b33fa22 to 981dce8 Compare July 25, 2024 09:23
@jaugustin
Copy link
Contributor Author

The failing test in code coverage is flaky, like on 1.x branch

@jaugustin
Copy link
Contributor Author

@kbond @nikophil Ican you review the MR, and tell me if more work is needed ?

@kbond
Copy link
Member

kbond commented Jul 29, 2024

Thanks for finding and fixing this issue @jaugustin! Just a question: with this PR, is there still value for the serialization test in TestTransport::send()? I'm thinking if you aren't actually processing the messages this is still valuable but I'd appreciate your thoughts!

@jaugustin
Copy link
Contributor Author

Thanks for finding and fixing this issue @jaugustin! Just a question: with this PR, is there still value for the serialization test in TestTransport::send()? I'm thinking if you aren't actually processing the messages this is still valuable but I'd appreciate your thoughts!

I think removing the test in the send part could be a breaking change, if devs rely on testing serialization only by sending a message and not processing it.

In a next major version maybe remove it from the send part, or do a bigger refactoring to allow to store the serialized version in a queue, that way, we test the serialization on sending and the deserialization on processing, to be more align with real transport.

If I take the pragmatic way, leave it, 2 tests is better than one ;)

@kbond kbond merged commit 07ea68a into zenstruck:1.x Jul 31, 2024
16 of 17 checks passed
@kbond
Copy link
Member

kbond commented Jul 31, 2024

Thanks @jaugustin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants