fix(test): update tests for entity class refactoring#623
Conversation
The entity classes (Struct, Class, Function) were removed in commit 19231c7 but the tests were not updated. Also replaced GitPython with pygit2 in test_git_history.py to match the project dependency. Changes: - test_c_analyzer.py: use FalkorDB Node property comparisons - test_py_analyzer.py: use FalkorDB Node property comparisons - test_git_history.py: replace GitPython with pygit2 API - test_graph_ops.py: use add_entity() and Node properties - api/graph.py: fix get_file() to return Node (consistent with other getters; old code called deleted File constructor) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR refactors the graph API to return raw FalkorDB Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Updates the test suite and a Graph helper to align with the recent removal/refactor of entity classes, moving tests toward validating FalkorDB Node properties and swapping Git history tests from GitPython to pygit2.
Changes:
- Update analyzer and graph ops tests to stop importing deleted entity classes and assert against
Node.properties. - Replace GitPython
Repousage withpygit2.Repositoryin git history tests. - Change
Graph.get_file()to return the underlying FalkorDBNodeinstead of constructing a deletedFileentity.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_py_analyzer.py | Replaces Class/Function entity comparisons with Node.properties assertions. |
| tests/test_c_analyzer.py | Replaces Struct/Function entity comparisons with Node.properties assertions. |
| tests/test_git_history.py | Migrates repository checkout/commit traversal from GitPython to pygit2. |
| tests/test_graph_ops.py | Uses Graph.add_entity() and Node.properties assertions; updates file creation path handling. |
| api/graph.py | Updates get_file() to return a FalkorDB Node and adjusts its docstring/type. |
Comments suppressed due to low confidence (1)
api/graph.py:444
- The docstring/arg naming here is ambiguous about what
pathshould contain.add_file()storespath=str(file.path)(full file path), but other call sites (e.g. coverage processing) often treatpathas the directory. Please clarify in the docstring (and example) whetherpathis expected to be the full file path or the directory, and alignget_file()/add_file()semantics accordingly to avoid hard-to-debug mismatches.
def get_file(self, path: str, name: str, ext: str) -> Optional[Node]:
"""
Retrieves a File node from the graph database based on its path, name,
and extension.
Args:
path (str): The file path.
name (str): The file name.
ext (str): The file extension.
Returns:
Optional[Node]: The File node if found, otherwise None.
Example:
file = self.get_file('/path/to/file', 'filename', '.py')
"""
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -24,37 +25,50 @@ def test_analyzer(self): | |||
| analyzer.analyze_local_folder(path, g) | |||
|
|
|||
| f = g.get_file('', 'src.py', '.py') | |||
There was a problem hiding this comment.
Graph.add_file() stores the File node with path=str(file.path) (full file path, including filename), so calling g.get_file('', 'src.py', '.py') will return None on a clean graph. Update this test to pass the same path value that gets persisted (e.g., the full path to src.py, or adjust Graph to store directory paths consistently).
| f = g.get_file('', 'src.py', '.py') | |
| src_file_path = os.path.join(path, 'src.py') | |
| f = g.get_file(src_file_path, 'src.py', '.py') |
| self.assertEqual(log.properties['name'], 'log') | ||
| self.assertEqual(log.properties['path'], 'src.py') | ||
| self.assertEqual(log.properties['ret_type'], 'None') | ||
| self.assertEqual(log.properties['src_start'], 0) | ||
| self.assertEqual(log.properties['src_end'], 1) | ||
| self.assertEqual(log.properties['args'], [['msg', 'str']]) |
There was a problem hiding this comment.
These assertions assume metadata like ret_type and args are present on Function nodes and that path == 'src.py'. In the current analysis pipeline, entities are created via graph.add_entity(..., props={}) (no ret_type/args), and path is set from str(file.path) (typically an absolute path). This will raise KeyError/fail in a fresh DB. Consider asserting only on guaranteed properties (name, path, src_start, src_end, doc) or updating the analyzer to populate these properties before asserting on them.
| f = g.get_file('', 'src.c', '.c') | ||
| self.assertEqual(File('', 'src.c', '.c'), f) | ||
| self.assertIsNotNone(f) | ||
| self.assertEqual(f.properties['name'], 'src.c') | ||
| self.assertEqual(f.properties['ext'], '.c') |
There was a problem hiding this comment.
Graph.add_file() persists file nodes with path=str(file.path) (full file path). Using g.get_file('', 'src.c', '.c') will not match that representation on a clean graph. Update the test to query using the same stored path value (or change the storage/query convention so path is directory-only and name is basename).
| s = g.get_struct_by_name('exp') | ||
| expected_s = Struct('src.c', 'exp', '', 9, 13) | ||
| expected_s.add_field('i', 'int') | ||
| expected_s.add_field('f', 'float') | ||
| expected_s.add_field('data', 'char[]') | ||
| self.assertEqual(expected_s, s) | ||
| self.assertIsNotNone(s) | ||
| self.assertEqual(s.properties['name'], 'exp') | ||
| self.assertEqual(s.properties['path'], 'src.c') |
There was a problem hiding this comment.
This test expects Struct/C Function nodes to exist after SourceAnalyzer.analyze_local_folder(path_to_c_sources, g), but the current SourceAnalyzer only enumerates *.java, *.py, and *.cs files (so no C files are analyzed and get_struct_by_name('exp') will be None on a clean graph). Either re-enable C support in the analyzer pipeline or skip/adjust this test to use a supported language.
Summary
Fix 3 test collection errors caused by importing deleted entity classes (
Struct,Class,Function) and using GitPython instead of pygit2.Changes
Struct/Functionimports with FalkorDB Node property comparisonsClass/Functionimports with FalkorDB Node property comparisonsfrom git import Repo(GitPython) withpygit2.Repositoryadd_entity()and Node properties instead of deleted classesget_file()to returnNode(was calling deletedFile(path, name, ext)constructor)Testing
pytest --collect-onlynow collects all 28 tests (was 3 errors blocking all tests)Memory / Performance Impact
N/A
Related Issues
Entity classes were removed in commit 19231c7 but tests were not updated.
Summary by CodeRabbit