Repository navigation
add tests for transitive_closure - #3703
Conversation
There was a problem hiding this comment.
Pull request overview
Adds unit tests for nltk.util.transitive_closure, improving coverage for directed-graph closure behavior and guarding against regressions in edge cases.
Changes:
- Import
transitive_closureinto the existing util unit test module. - Add tests for chain closure, reflexive closure, cycle handling/termination, and empty graphs.
- Add a test intended to ensure the input graph is not mutated.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def test_transitive_closure_does_not_mutate_input(): | ||
| graph = {"a": {"b"}, "b": set()} | ||
| transitive_closure(graph) | ||
| assert graph == {"a": {"b"}, "b": set()} |
|
Hi @RavSinghChandan, what do you think about the unresolved review concern? |
|
Good catch — you're right that value equality alone doesn't pin this. Updated the test to hold on to the original set objects and assert identity Checked it actually bites: patching transitive_closure to rebind the caller's |
ekaf
left a comment
ekaf
left a comment
There was a problem hiding this comment.
The latest commit fully addresses the review concern about missing coverage for transitive_closure: it adds concrete tests for chain propagation, reflexive closure, cycles, empty graph behavior, and input non-mutation/object identity, all in nltk/test/unit/test_util.py. This is a high-quality test-only PR with clear regression value and no runtime risk, and while it is not urgent like a security fix, it is still important to merge because it hardens a core utility against silent future breakage.
noticed
transitive_closureinnltk/util.pyhas no unit tests, so added a few totest_util.pynext to the existing everygrams ones.covers the cases i cared about while reading it:
reflexive=Truepath (each node includes itself)all green locally, pre-commit passes.