-
Notifications
You must be signed in to change notification settings - Fork 34
test_schemaview.py: split out long test method into many tests #456
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: schemaview_explode_tests
Are you sure you want to change the base?
test_schemaview.py: split out long test method into many tests #456
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## schemaview_explode_tests #456 +/- ##
=========================================================
Coverage 77.37% 77.37%
=========================================================
Files 52 52
Lines 4478 4478
Branches 979 979
=========================================================
Hits 3465 3465
Misses 785 785
Partials 228 228 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
assert view.permissible_value_parent(pv, e.name) == ["LION"] | ||
assert view.permissible_value_ancestors(pv, e.name) == ["ANGRY_LION", "LION", "CAT"] | ||
assert view.permissible_value_descendants(pv, e.name) == ["ANGRY_LION"] | ||
animals = "Animals" |
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.
made these tests more comprehensive and got rid of the unnecessary iteration / nesting
assert set(e.include[0].reachable_from.source_nodes) == {"GO:0007049", "GO:0022403"} | ||
|
||
|
||
def test_schemaview(schema_view_no_imports: SchemaView) -> None: |
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 test is far too long and tests too many different things. It is better to keep tests short and focused on specific, related methods. I went through the method and split it up as logically as I could.
logger.debug(view.imports_closure()) | ||
assert len(view.imports_closure()) == 1 |
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.
previous PR has a better test of the imports closure
all_cls = view.all_classes() | ||
logger.debug(f"n_cls = {len(all_cls)}") |
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.
least favourite test anti-pattern: printing out stuff and not doing any testing on it.
with pytest.raises(ValueError, match='property to introspect must be of type "boolean"'): | ||
view.slot_is_true_for_metadata_property("aliases", "aliases") | ||
|
||
for tn, t in view.all_types().items(): |
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.
tested more thoroughly elsewhere - e.g. see the creature schema tests
for sn, s in view.all_slots().items(): | ||
logger.info(f"SN = {sn} RANGE={s.range}") |
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.
again, tested better elsewhere
|
||
assert view.get_uri(COMPANY) == "ks:Company" | ||
|
||
# test induced slots |
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.
move all these induced slot tests down to their own induced slot section
assert view.class_slots(PERSON) == view.class_slots(ADULT) | ||
assert set(view.class_slots(COMPANY)) == {"id", "name", "ceo", "aliases"} | ||
|
||
assert view.get_class(AGENT).class_uri == "prov:Agent" |
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.
we already have these tests for get_uri and class_uri in another method - no need to repeat them here.
Just as it says in the title -- splitting out long SchemaView tests by the functions that they are testing.
See inline comments for details.