-
Notifications
You must be signed in to change notification settings - Fork 820
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 tests for all Serializable classes #631
Comments
For example, I just discovered that AsGraphUnion is not (by default) serializable, since the out-of-the-box lambdas produced by WeightCombiner are not marked as serializable. (It's possible to work around by supplying a serializable WeightCombiner implementation.) |
Can I start working on this? |
Yes, thanks. It would be best to start with one PR which accomplishes:
Once that is merged, follow up with additional PR's for each logical module, as you suggest. |
Thanks, I've merged it...you can go ahead with adding test coverage now. Let's see how many bugs you find :) |
@jsichi Thanks, I'll start working on writing serialization tests. |
@jsichi I've created a new PR#780 Kindly review it. Thanks. |
Hey @LavishKothari Great work so far! Are you planning to continue with all of the other serializable data structure/algorithm classes? |
Yes @jsichi I'm planning to work continue working on this. Apologies as I was busy. |
I grepped for "Serializable" and then deleted stuff which is already covered by existing tests:
|
It's very easy to mark a class Serializable, but it only works if the transitive closure of all referenced classes are also Serializable. The only way to verify we got this right (at least for the default choice of class instantiations) is via unit tests which serialize and then deserialize. Currently, we only have a few of these:
We should factor out the common test infrastructure for this (it's only a few lines of code), and then
The text was updated successfully, but these errors were encountered: