-
Notifications
You must be signed in to change notification settings - Fork 2
Add GraphsInterfaceChecker compliance tests #12
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12 +/- ##
=======================================
Coverage 80.00% 80.00%
=======================================
Files 1 1
Lines 20 20
=======================================
Hits 16 16
Misses 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for starting this! There are a few issues, but chiefly, make sure that your tests actually run before submitting them. It seems you are using a coding agent as well -- that is fine, it is a useful tool, but you have to disclose this as it informs how to approach review (coding agents make different, frequently very subtle mistakes). We can also plan to prepare a CLAUDE.md file to help you with this.
[deps] | ||
Aqua = "4c88cf16-eb10-579e-8560-4a9242c79595" | ||
Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6" | ||
GraphsInterfaceChecker = "3bef136c-15ff-4091-acbb-1a4aafe67608" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GraphsInterfaceChecker is not registered so this will have to be a manually installed dependency (using Pkg inside of the runtests.jl). See https://github.com/JuliaGraphs/NautyGraphs.jl/pull/25/files#diff-060d04ab60a4aa7e86bfb99aaf52194f3248724e7b254508b6e7106e354101da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops, my comment is wrong, it seems GraphsInterfaceChecker is actually registered, you can disregard this comment
Graphs = "86223c79-3864-5bf0-83f7-82e725a168b6" | ||
GraphsInterfaceChecker = "3bef136c-15ff-4091-acbb-1a4aafe67608" | ||
JET = "c3a54625-cd67-489e-a8e7-0a5a0ff4e31b" | ||
LEMONGraphs = "14b1564f-c77f-4800-9e89-efd961faef7c" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LEMONGraphs is already the parent project -- it should not be added here.
if hasmethod(check_mutable_graph_interface, Tuple{typeof(g)}) | ||
check_mutable_graph_interface(g) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reads a bit like it was confabulated by an LLM. LLMs are useful tools, but you have to disclose their use, as that informs how to perform the review.
hasmethod
is not particularly idiomatic piece of code -- usually one just does dispatch. In this particular case, the agent you used probably usedhasmethod
to ensure that the next line is never executed. That is typical of AI agents -- they write wrong code and then wrap it in anif
so that the wrong code never runs- the symbol
check_mutable_graph_interface
does not exist anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look at NautyGraphs for how to structure this file
end | ||
end | ||
|
||
@testset "Directed LEMONGraph" begin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can skip this for now -- let's make sure the undirected graph case works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is not seen by the test runner. If you use @testset
then it needs to be included in runtests.jl
. We actually use TestItems.jl
here to make this a bit more automated, in which case you need @testitem
.
This PR adds a new test file, test/test_interface_checker.jl, to verify that LEMONGraphs.jl correctly implements the Graphs.jl API. It uses the GraphsInterfaceChecker.jl package to automatically validate compliance for both directed and undirected LEMON graph types.
Added test/test_interface_checker.jl, which contains :