Add unit tests of lookup functionality and minor lookup refactor#80
Conversation
Codecov Report
@@ Coverage Diff @@
## main #80 +/- ##
===========================================
+ Coverage 61.55% 77.83% +16.27%
===========================================
Files 23 23
Lines 1514 1877 +363
===========================================
+ Hits 932 1461 +529
+ Misses 582 416 -166
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
What exactly is stuck here, do we know that? Can I help? |
@fkiraly I haven't investigated the POSIX paths not having starts with attribute in detail yet. Those aren't occurring locally. I'm checking now if the issue is with the validation of the returned results in the test or call to the actual _walk function having some sort of bug. Will let you know. Other errors are just things popping up as I am slowly adding tests (I elected to keep adding tests and let the failing test hang out till I was finished). Expect to finish up the test adding today or tomorrow. |
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Great!
- tests make sense overall. Impressed that we tests internal utilities as well as the full function.
- tests seem to cover important arguments of
all_objects - left some minor/non-blocking comments overall
My main blocking comment or question:
- overall, I don't see a test that compares retrieved objects with a hard-coded set of expected objects from the mock package, e.g., in
test_all_objects_returns_expected_types. Maybe I'm just blind/confused test_all_object_class_lookup, should this not test the content of the return,objs, being the right objects (given the mock package)?
That's a good point. I'd been avoiding a specific comparison and instead verifying things based on whether the return types, counts, etc make sense in light of the filters being applied on the returned metadata/objects. I'll push a test of the specific output as well. I'll make that separate from the |
|
|
||
|
|
||
| # Fixture class for testing tag system | ||
| class Parent(BaseObject): |
Check warning
Code scanning / CodeQL
`__eq__` not overridden when adding attributes
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
Looks good to me now.
Checked:
- reference in
TestAllObjects - mock package looks good
- lookup also uses mock package
All comments addressed, hence.
4 participants
Reference Issues/PRs
Fixes #70, improves test coverage (fixes #66), fixes bugs, improves code consistency and docstrings (fixes #35 ).
What does this implement/fix? Explain your changes.
This adds unit tests for the lookup functionality in
skbase. Also does minor refactor and docstring improvements as things are discovered in testing process.Does your contribution introduce a new dependency? If yes, which one?