Jgrapht: add tests for all Serializable classes

Created on 14 Jul 2018  路  12Comments  路  Source: jgrapht/jgrapht

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
cleanup gsoc

All 12 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?
Is it required that I created separate PRs for different logical modules so that reviewing is easy?

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.

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

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.

@jsichi I've created a new PR#781. 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 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.

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

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.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

hulk-baba picture hulk-baba  路  13Comments

nikhilbhardwaj picture nikhilbhardwaj  路  3Comments

jsichi picture jsichi  路  12Comments

haerrel picture haerrel  路  5Comments

io7m picture io7m  路  53Comments