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

Serialization testing - symmetric assertions to the unit tests #261

Closed
greg-szabo opened this issue May 8, 2020 · 5 comments · Fixed by #283
Closed

Serialization testing - symmetric assertions to the unit tests #261

greg-szabo opened this issue May 8, 2020 · 5 comments · Fixed by #283
Assignees

Comments

@greg-szabo
Copy link
Member

greg-szabo commented May 8, 2020

Current unit tests for the serialization library separates serialization and deserialization of the same struct into separate tests. These could go into one test.

Relevant discussions:
yihuang: symmetric testing
liamsi: function from Romain
tarcieri: proptest
Another take from Ismail

@greg-szabo
Copy link
Member Author

Note from Rust developers: serde-rs/serde#1690 (comment)

Be aware that neither serde nor serde_json makes round trip guarantees in general.

@liamsi
Copy link
Member

liamsi commented May 13, 2020

I think for some types not having the possibility to read what we wrote can cause problems (less problematic are edge cases like null or omitted fields we get from tendermint go). I think @yihuang and maybe @romac stumbled upon this (I think sth. we persisted by serialzing in the context if the light client couldn't properly be deserialized again; don't remeber the details). Roundtripping prevents bugs like this. So maybe @romac's rountrip method is too strict for some cases (like null etc), not sure, but we need to guarantee that we can go from T->JSON->T (and that both Ts match) at least.

@greg-szabo greg-szabo self-assigned this May 26, 2020
@greg-szabo
Copy link
Member Author

greg-szabo commented May 26, 2020

we need to guarantee that we can go from T->JSON->T (and that both Ts match)

I'm highlighting this as the core summary of the issues.

@greg-szabo
Copy link
Member Author

The opened PR fixes this in the serialization unit tests that originally spawned the issue. I'm sure there are a lot of other places to make fixes but that should require a different issue (more like a porject) with an overarching goal. This is more in line with fixing the comments that fell out of previous PRs.

Also, I'm trying to close open PRs and #98 from @yihuang seemed a good fit for this issue too. Hence his changes were reimplemented here (too many changes on master for a simple cherry-picking, unfortunately).

@yihuang , thanks for your patience regarding your change and feel free to chime in on this one, if you feel like it.

@greg-szabo
Copy link
Member Author

Additional note: proptest, mentioned by Tony seems more in line with a "project-like" issue where we overhaul how we're doing tests. It seemed overkill for this small fix, but I'm happy to open a follow-up issue that goes through our testing and implements better ways for testing.
I just don't see it as priority right now.

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 a pull request may close this issue.

2 participants