Graphs.jl
Graphs.jl copied to clipboard
Add generic graphs to help with testing
Our tests currently almost everywhere use SimpleGraph
and SimpleDiGraph
for testing methods that work with AbstractGraph
and AbstractEege
.. This can lead to issues, when the methods under test relay on methods that are defined for SimpleGraph
and SimpleDiGraph
, but are not part of the Graphs.jl interface.
This PR therefore adds a submodule Graphs.Test
and the generic graph types GenericGraph
and GenericDiGraph
as well as the generic edge type GenericEdge
that correspond to the interface, but nothing else. The submodule is located within the src
and not the test
directory, so that it can also be used in tests of downstream packages.
The names are inspired from similar structures such as GenericArray
and GenericOrder
in the Julia package Test.jl
.
Mutability is currently not covered, as the interface is also not well defined there - this is something we have to tackle in the future.
I also added the generic types to a few existing tests to demonstrate their functionality - in the future we should also modify other tests to use them.
Codecov Report
Merging #133 (000136b) into master (897e183) will decrease coverage by
0.13%
. The diff coverage is50.00%
.
:exclamation: Current head 000136b differs from pull request most recent head 7970783. Consider uploading reports for the commit 7970783 to get more accurate results
@@ Coverage Diff @@
## master #133 +/- ##
==========================================
- Coverage 97.40% 97.27% -0.14%
==========================================
Files 109 110 +1
Lines 6470 6377 -93
==========================================
- Hits 6302 6203 -99
- Misses 168 174 +6
@etiennedeg @gdalle can I have your opinion on that? I think it is something that would be quite useful to have.
@simonschoelly I think it's a really good idea, and it goes some of the way in clarifying interface specification.
If I understand correctly, these types are just wrappers around a SimpleGraph
/SimpleEdge
, right? So we'd only be testing whether functions accept the correct (generic) types, but not whether their behavior is correct.
That's already significant progress, but your PR makes me wonder whether we could also create a pathological graph type that satisfies the interface but will explode if we try anything fishy with it.
I also think it's good to have.
Regarding your comment on edges, I also think it would be good to have a distinction between directed on non-directed edges, but we should expect the same behavior for reverse
on edges returned by a non-directed graph, otherwise, this will be a mess. A discussion for 2.0 ?
@simonschoelly I think it's a really good idea, and it goes some of the way in clarifying interface specification. If I understand correctly, these types are just wrappers around a
SimpleGraph
/SimpleEdge
, right? So we'd only be testing whether functions accept the correct (generic) types, but not whether their behavior is correct. That's already significant progress, but your PR makes me wonder whether we could also create a pathological graph type that satisfies the interface but will explode if we try anything fishy with it.
Actually, as these wrappers should correspond correctly to the interface, using them in tests does ensure that these tests work correctly. And these wrappers will fail if one does something fishy with it, for example trying to access g.fadjlist
.
I'm in favor of merging, and I opened #224 to keep track of the transition