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

Add msgpack to the crypto fixture data #10

Merged
merged 1 commit into from
Oct 6, 2015
Merged

Conversation

SimonWoolf
Copy link
Member

@paddybyers
Copy link
Member

any objections?

No, but right now we don't know how you're going to use these in tests.

It's not valid, unfortunately, to assume key order in the msgpack serialisation, so you can't do an encode() and compare the text output.

Verification of the decode() can easily be by decoding the msgpack string and doing a deepEqual on the result.

Verification of the encode() strictly would need us to have an independent and valid decoder to decode the output of the client library, and then do a deepEqual on the results.

@SimonWoolf
Copy link
Member Author

It's not valid, unfortunately, to assume key order in the msgpack serialisation,

That's annoying. A combination of decoding the fixture and comparing with deepEqual, plus comparing the results of msgpack-encoding the testMessage and the encryptedMessage, would hopefully be enough to catch most bugs (the latter is enough to catch the bug we just fixed), but still, would have been nice to compare the result of encode() directly. Oh well.

@SimonWoolf
Copy link
Member Author

(Aaand I've just realised all the weird behaviour I've been debugging for the past hour is because I generated these without passing sparse: true, and the msgpack decoder can't handle keys with undefined values. headdesk. Will regenerate)

SimonWoolf added a commit that referenced this pull request Oct 6, 2015
Add msgpack to the crypto fixture data
@SimonWoolf SimonWoolf merged commit d73d700 into master Oct 6, 2015
@QuintinWillison QuintinWillison deleted the msgpack-fixtures branch July 26, 2021 08:19
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