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 tests for all Serializable classes #631

Open
jsichi opened this issue Jul 14, 2018 · 12 comments
Open

add tests for all Serializable classes #631

jsichi opened this issue Jul 14, 2018 · 12 comments
Labels
cleanup gsoc candidate task for Google Summer of Code

Comments

@jsichi
Copy link
Member

jsichi commented Jul 14, 2018

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:

  • org.jgrapht.graph.SerializationTest for default graph implementations
  • org.jgrapht.graph.guava (comprehensive using SerializationTestUtils)

We should factor out the common test infrastructure for this (it's only a few lines of code), and then

  • add serialize+deserialize unit tests for all existing Serializable classes
  • make this a requirement when reviewing new pull requests
@jsichi jsichi added the cleanup label Jul 14, 2018
@jsichi jsichi mentioned this issue Jul 14, 2018
7 tasks
@jsichi jsichi added the gsoc candidate task for Google Summer of Code label Oct 9, 2018
@jsichi
Copy link
Member Author

jsichi commented Apr 22, 2019

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.)

@LavishKothari
Copy link

Can I start working on this?
Is it required that I created separate PRs for different logical modules so that reviewing is easy?

@jsichi
Copy link
Member Author

jsichi commented May 6, 2019

Yes, thanks. It would be best to start with one PR which accomplishes:

  • move SerializationTestUtils form jgrapht-guava to jgrapht-core
  • change existing SerializationTest to use SerializationTestUtils
  • change existing guava tests to use SerializationTestUtils (via test->test dependency on jgrapht-core)

Once that is merged, follow up with additional PR's for each logical module, as you suggest.

@LavishKothari
Copy link

@jsichi Thanks for suggesting the starting point.
I've created the PR#779

@jsichi
Copy link
Member Author

jsichi commented May 7, 2019

Thanks, I've merged it...you can go ahead with adding test coverage now. Let's see how many bugs you find :)

@LavishKothari
Copy link

@jsichi Thanks, I'll start working on writing serialization tests.

@LavishKothari LavishKothari mentioned this issue May 8, 2019
7 tasks
@LavishKothari
Copy link

@jsichi I've created a new PR#780 Kindly review it. Thanks.

@LavishKothari
Copy link

@jsichi I've created a new PR#781. Kindly review it.
Thanks.

@jsichi
Copy link
Member Author

jsichi commented May 31, 2019

Hey @LavishKothari

Great work so far! Are you planning to continue with all of the other serializable data structure/algorithm classes?

@LavishKothari
Copy link

Yes @jsichi I'm planning to work continue working on this. Apologies as I was busy.
I will try to figure out what's the next set of data-structure/algorithm that I can take up for the serialization tests, but if you have anything in mind that will help me a lot.
Thanks.

@jsichi
Copy link
Member Author

jsichi commented Jun 1, 2019

I grepped for "Serializable" and then deleted stuff which is already covered by existing tests:

jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/CapacitatedSpanningTreeAlgorithm.java:    class CapacitatedSpanningTreeImpl<V, E> implements CapacitatedSpanningTree<V, E>, Serializable {
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/CycleBasisAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/MatchingAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/PartitioningAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/SpannerAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/SpanningTreeAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/TreeToPathDecompositionAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/interfaces/VertexColoringAlgorithm.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/shortestpath/ListMultiObjectiveSingleSourcePathsImpl.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/shortestpath/ListSingleSourcePathsImpl.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/alg/shortestpath/TreeSingleSourcePathsImpl.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/AsGraphUnion.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/AsSubgraph.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/AsUndirectedGraph.java:    Serializable,
jgrapht-core/src/main/java/org/jgrapht/graph/AsUnmodifiableGraph.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/AsUnweightedGraph.java:    Serializable,
jgrapht-core/src/main/java/org/jgrapht/graph/AsWeightedGraph.java:    Serializable,
jgrapht-core/src/main/java/org/jgrapht/graph/DefaultGraphType.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/DirectedAcyclicGraph.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/GraphWalk.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/MaskEdgeSet.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/MaskSubgraph.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/MaskVertexSet.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/graph/concurrent/AsSynchronizedGraph.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/util/SupplierUtil.java:        Serializable
jgrapht-core/src/main/java/org/jgrapht/util/UnmodifiableUnionSet.java:    Serializable
jgrapht-core/src/main/java/org/jgrapht/util/WeightedUnmodifiableSet.java:    Serializable
jgrapht-io/src/main/java/org/jgrapht/io/DefaultAttribute.java:    Serializable
jgrapht-opt/src/main/java/org/jgrapht/opt/graph/fastutil/FastutilGSS.java:            Specifics<V, E>> & Serializable) (graph, type) -> {

@CharulBhanawat13
Copy link

I have created a new PR #789.This PR contains serialization test for AsGraphUnion. This test was initially failing, I have made WeightCombiner serializable to make AsGraphUnion serializable.

@jsichi Kindly review this and please let me know if there are any comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup gsoc candidate task for Google Summer of Code
Projects
None yet
Development

No branches or pull requests

3 participants