UI server test coverage & ARGS_SPEC Workbench compatibility #623
Conversation
… a python script. natcap#617.
…VER-617-test-coverage
…s to format pint.Unit instances. natcap#617.
…VER-617-test-coverage
…ch model ARGS_SPEC. natcap#617.
…me in each model ARGS_SPEC. natcap#617." This reverts commit 23754be.
src/natcap/invest/ui_server.py
Outdated
| @@ -25,7 +22,7 @@ | |||
| v.pyname: _UI_META( | |||
| run_name=k, | |||
| human_name=v.humanname) | |||
| for k, v in natcap.invest.cli._MODEL_UIS.items()} | |||
| for k, v in cli._MODEL_UIS.items()} | |||
There was a problem hiding this comment.
To be nitpicky, key and value or val instead of one-letter variables?
| try: | ||
| _ = spec_utils.serialize_args_spec(model.ARGS_SPEC) | ||
| except TypeError: | ||
| self.fail( |
There was a problem hiding this comment.
Neat, I haven't seen self.fail before! Could we also show the original error traceback?
There was a problem hiding this comment.
Yeah self.fail is new to me too. This seems like the preferred pattern to assert that a specific exception is avoided.
Good idea adding the original error. It's hard (but not impossible) to get the full traceback without ever raising the error. And this TypeError should always be the one explicitly raised by spec_utils.fallback_serializer so I think it's okay to just include that message without traceback. An example with these changes:
E AssertionError: Failed to avoid TypeError when serializing natcap.invest.carbon.ARGS_SPEC:
E fallback serializer is missing for <class 'module'>
There was a problem hiding this comment.
Oh interesting. I don't usually think about making that distinction between a test failure and (unexpected) error.
tests/test_ui_server.py
Outdated
| response_data = json.loads(response.get_data(as_text=True)) | ||
| self.assertEqual( | ||
| list(response_data), | ||
| ['type', 'args', 'module_name', 'model_run_name', |
There was a problem hiding this comment.
It looks like the json module preserves the order of keys, so this should work. But JSON is generally unordered outside of the python implementation, so I think ui_server.post_datastack_file could put them in any order without breaking its contract. Maybe use a set here just in case?
There was a problem hiding this comment.
Great point! I changed all these tests to use sets instead of lists.
tests/test_ui_server.py
Outdated
| actual_data = json.loads(file.read()) | ||
| self.assertEqual( | ||
| list(actual_data), | ||
| ['args', 'invest_version', 'model_name']) |
There was a problem hiding this comment.
like above, maybe should be a set? If we do want to guarantee the order of keys in the file produced by write_parameter_set_file, lets put that in the docstring
There was a problem hiding this comment.
order shouldn't matter; changed to a set.
tests/test_ui_server.py
Outdated
| spec = json.loads(response.get_data(as_text=True)) | ||
| self.assertEqual( | ||
| list(spec), | ||
| ['model_name', 'module', 'userguide_html', |
tests/test_ui_server.py
Outdated
| response = test_client.get('/models') | ||
| models_dict = json.loads(response.get_data(as_text=True)) | ||
| for model in models_dict.values(): | ||
| self.assertEqual(list(model), ['internal_name', 'aliases']) |
This PR makes small modifications to
ARGS_SPECoptionsdatatypes - changing from sets to lists - and adds a utility function to make sure allARGS_SPECdictionaries are JSON serializable.There are also new tests for all endpoints in
ui_server.py. And tests forspec_utilsfunctions were moved intotest_args_spec.py.Fixes #617
Checklist
Updated HISTORY.rst (if these changes are user-facing)
Updated the user's guide (if needed)
The corresponding workbench PR that depends on these changes is natcap/invest-workbench#165