Graphs.jl icon indicating copy to clipboard operation
Graphs.jl copied to clipboard

Add generic graphs to help with testing

Open simonschoelly opened this issue 2 years ago • 4 comments

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.

simonschoelly avatar May 22 '22 17:05 simonschoelly

Codecov Report

Merging #133 (000136b) into master (897e183) will decrease coverage by 0.13%. The diff coverage is 50.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     

codecov[bot] avatar May 22 '22 17:05 codecov[bot]

@etiennedeg @gdalle can I have your opinion on that? I think it is something that would be quite useful to have.

simonschoelly avatar Jul 24 '22 14:07 simonschoelly

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

gdalle avatar Jul 24 '22 14:07 gdalle

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 ?

etiennedeg avatar Jul 28 '22 09:07 etiennedeg

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

simonschoelly avatar Sep 24 '22 11:09 simonschoelly

I'm in favor of merging, and I opened #224 to keep track of the transition

gdalle avatar Feb 09 '23 12:02 gdalle