Refactor Schemas: unit tests FDS-1061 TestDataModelNodes #1310
Conversation
| # Test adding a dummy node | ||
| node_dict = {'label': 'test_label'} | ||
|
|
||
| path_to_data_model = helpers.get_data_path(data_model) |
There was a problem hiding this comment.
actually probably don't need path_to_data_model here.
| path_to_data_model = helpers.get_data_path(data_model) | ||
|
|
||
| # Get Graph | ||
| graph_data_model = generate_graph_data_model(helpers, data_model_name=path_to_data_model) |
There was a problem hiding this comment.
just pass data_model here.
|
|
||
| assert nodes == expected_nodes | ||
|
|
||
| # Ensure order is tested. |
There was a problem hiding this comment.
I am a bit confused here. Here it seems like we are ensuring "Patient" does not become the last item in a list?
There was a problem hiding this comment.
This is an additional maybe unnecessary test to ensure that the order and not just the contents is being tested in the previous test. this is sort of a future proof thing...
| reordered_nodes.append('Patient') | ||
| assert reordered_nodes != expected_nodes | ||
|
|
||
| @pytest.mark.parametrize("data_model", list(DATA_MODEL_DICT.keys()), ids=list(DATA_MODEL_DICT.values())) |
There was a problem hiding this comment.
Could be re-written like:
@pytest.mark.parametrize("data_model", list(DATA_MODEL_DICT.keys()), ids=list(DATA_MODEL_DICT.values())) class TestDataModelNodes:
| assert reordered_nodes != expected_nodes | ||
|
|
||
| @pytest.mark.parametrize("data_model", list(DATA_MODEL_DICT.keys()), ids=list(DATA_MODEL_DICT.values())) | ||
| def test_gather_all_nodes(self, helpers, data_model): |
There was a problem hiding this comment.
I understand what you are trying to do here. I am thinking since gather_all_nodes calls gather_nodes function, will it make sense to mock gather_nodes and then test if gather_all_nodes could return the correct output?
| data_model_properties = data_model_nodes.get_data_model_properties(attr_rel_dictionary) | ||
|
|
||
| # In the current example model, there are no properties, would need to update this section if properties are added. | ||
| assert data_model_properties == [] |
There was a problem hiding this comment.
Yeah, this is a good point. I created a ticket here to track: https://sagebionetworks.jira.com/jira/software/c/projects/FDS/issues/FDS-1226
| # In the example data model all attributes should be classes. | ||
| for attr in attr_rel_dictionary.keys(): | ||
| entry_type = data_model_nodes.get_entry_type(attr) | ||
| assert entry_type == 'class' |
There was a problem hiding this comment.
TODO: after adding an example data dictionaries that have properties, we could change this part of the test
| @pytest.mark.parametrize("rel_func", list(REL_FUNC_DICT.values()), ids=list(REL_FUNC_DICT.keys())) | ||
| @pytest.mark.parametrize("test_dn", list(TEST_DN_DICT.keys()), ids=list(TEST_DN_DICT.keys())) | ||
| @pytest.mark.parametrize("test_bool", ['True', 'False', True, False, 'kldjk'], ids=['True_str', 'False_str', 'True_bool', 'False_bool', 'Random_str']) | ||
| def test_run_rel_functions(self, helpers, data_model, rel_func, test_dn, test_bool): |
There was a problem hiding this comment.
Do we have unit tests for get_attribute_display_name_from_label, parse_validation_rules, get_label_from_display_name, convert_bool_to_str? If so, I think here we could simply mock responses from these four functions and test run_rel_functions.. Currently what you are doing here makes sense. But at the same time, I feel like this test becomes difficult to read because it gathers essentially four tests of four different functions
There was a problem hiding this comment.
we do have tests for those, but the trick here is actually making sure the parameters are being passed properly through this function.. which has at times felt a bit chaotic, so its nice to call the functions this way.
|
@mialy-defelice I left some comments for your reference. I think it general it looks good. Feel free to merge |
…-unit-tests-FDS-1061
No description provided.