-
Notifications
You must be signed in to change notification settings - Fork 2
Incorporate test interface.jl using GraphsInterfaceChecker #18
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
Incorporate test interface.jl using GraphsInterfaceChecker #18
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18 +/- ##
======================================
Coverage 6.48% 6.48%
======================================
Files 8 8
Lines 4332 4332
======================================
Hits 281 281
Misses 4051 4051 ☔ 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! These tests seem to test only the base array interface and to not actually do anything related to GraphsInterfaceChecker. These tests are already exist in the test folder -- it does not make sense to repeat them here.
Assuming this is related to the integration bounty, the changes here should be related to bringing in GraphsInterfaceChecker and the tests it provides. Do you think you can do that without an LLM? I believe the use of the LLM is just causing confusion and mistakes, steering you in an unproductive direction. If you can not do these tests without an LLM, I would like to cancel this bounty assignment.
using TestItemRunner | ||
|
||
|
||
include("interface.jl") # NEW |
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 is not necessary, we are using TestItemRunner.jl which picks tests up automatically as long as they are marked appropriately. Check out its docs.
println("Starting tests with $(Threads.nthreads()) threads out of `Sys.CPU_THREADS = $(Sys.CPU_THREADS)`...") | ||
|
||
@run_package_tests filter=testfilter | ||
@run_package_tests filter=testfilter |
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.
Your editor seems to have removed the ending newline in the file. Stack overflow has a good discussion about why ending newlines are important and why tools like github mark their omission with a warning sign. You can also set up your editor in a way that ensures that ending newlines are always present.
# test_interface_igraphs_containers.jl | ||
# Pattern inspired by NautyGraphs' interface test, adapted to test the | ||
# IGraphs container interfaces using GraphsInterfaceChecker. |
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.
Avoid superfluous comments. LLMs in particular generate a lot of unhelpful and occasionally misleading comments. If the code is obvious, there should be no comments.
using IGraphs | ||
|
||
@testset "interface (IGraphs base containers)" begin | ||
# === Vector-like interface checks (IGraphs.vtypes) === |
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.
These tests seem to already exist elsewhere in the code base. No need to repeat them. This seems to actually copy a lot of these existing tests.
I will close this PR as the LLM generated code here has issues. Let me know if you would like to discuss it again. |
Hello @Krastanov, I just make changes following your notes
PR Summary
This PR adds interface conformance tests for the IGraphs.jl vector and matrix container types, following the same testing pattern used in NautyGraphs.jl with
GraphsInterfaceChecker
.Changes
New test file:
test/interface.jl
@implements
clauses for vector (vtypes
) and matrix (mtypes
) container types from IGraphs.Interfaces.test
checks for:BaseInterfaces.IterationInterface
BaseInterfaces.IterationInterface{(:indexing,)}
BaseInterfaces.ArrayInterface
BaseInterfaces.ArrayInterface{(:logical,:setindex!)}
Vector ↔ VT
,Matrix ↔ MT
) based on IGraphs’ existingtest_interfaces.jl
.Test environment setup
Pkg.add
calls from the test file.Interfaces
,GraphsInterfaceChecker
) in thetest/Project.toml
.Pkg.test()
works without side effects).Motivation
Notes
interface.jl
to ensure consistency across repos.