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 generic graphs to help with testing #133

Merged

Conversation

simonschoelly
Copy link
Member

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 simonschoelly added the enhancement New feature or request label May 22, 2022
@simonschoelly simonschoelly self-assigned this May 22, 2022
@codecov
Copy link

codecov bot commented May 22, 2022

Codecov Report

Merging #133 (000136b) into master (897e183) will decrease coverage by 0.13%.
The diff coverage is 50.00%.

❗ 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     

@simonschoelly
Copy link
Member Author

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

@gdalle
Copy link
Member

gdalle commented Jul 24, 2022

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

@etiennedeg
Copy link
Member

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
Copy link
Member Author

@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 simonschoelly force-pushed the add-generic-graphs-for-testing branch from 019ab5b to 4152778 Compare September 24, 2022 11:24
@simonschoelly simonschoelly force-pushed the add-generic-graphs-for-testing branch from 000136b to 86b9f06 Compare December 18, 2022 10:52
@simonschoelly simonschoelly force-pushed the add-generic-graphs-for-testing branch from 86b9f06 to 7970783 Compare December 18, 2022 11:04
@gdalle gdalle mentioned this pull request Feb 9, 2023
12 tasks
@gdalle
Copy link
Member

gdalle commented Feb 9, 2023

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

@gdalle gdalle merged commit c660306 into JuliaGraphs:master Feb 9, 2023
@gdalle gdalle mentioned this pull request Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants