Skip to content

Standard errors#25

Merged
jayrbolton merged 2 commits intodevelopfrom
standard_errors
Sep 1, 2020
Merged

Standard errors#25
jayrbolton merged 2 commits intodevelopfrom
standard_errors

Conversation

@ialarmedalien
Copy link
Copy Markdown
Collaborator

@ialarmedalien ialarmedalien commented Aug 31, 2020

Standardised error responses from the API.

Also made misc fixes and added tests for various pieces of untested code.

Closes #14

  • I updated the README.md docs to reflect this change.
  • This is not a breaking API change

Add a couple more tests for error cases.
Add coverage calculation to test runs.

Update readme with error structure details
@ialarmedalien ialarmedalien self-assigned this Aug 31, 2020
Comment on lines +27 to +28
self.assertTrue(True)
return
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same effect as skipTest but without giving a misleading s in the test output

Comment thread scripts/run_tests.sh
python -m spec.validate &&
# spec stored query tests
python -m unittest discover spec/test &&
coverage run --parallel-mode -m unittest discover spec/test &&
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add in coverage calculation

Comment thread spec/test/djornl/results.json Outdated
"AT1G01050__AT1G01060__AraNetv2-LC_lit-curated-ppi__2.7",
"AT1G01080__AT1G01090__AraNetv2-LC_lit-curated-ppi__2.8"
]
"-": {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small reformat, big diff!

@@ -0,0 +1,64 @@
name: ncbi_taxon
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next four files are just to check that the validator picks up duplicate file names.

Comment thread spec/validate.py
Comment on lines +60 to +72
try:
data = validate_schema(path, schema_type)
# Check for any duplicate schema names
name = data['name']
if name in names:
raise ValueError(f"Duplicate queries named '{name}'")
else:
names.add(name)

except Exception as err:
print(f"✕ {path} failed validation")
print(err)
err_count += 1
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validate all files in the dir, don't quit after the first failure

Comment thread spec/validate.py
return


def validate_all_by_type(validation_base_dir=None):
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new method that pulls together all the validation routines

Comment thread spec/validate.py Outdated
Comment thread relation_engine_server/main.py Outdated

'error': {
'message': <text explanation of error>,
'status': <HTTP error code>,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like we need status here if it is always in the header.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that was my thought too - the only reason I left it in is that it was in several of the error structures. I'm happy to remove it globally.

@jayrbolton
Copy link
Copy Markdown
Contributor

A couple small comments above, otherwise looks good

Refactor test_djornl tests to put all results in the results.json file.
Update djornl_fetch_* queries to ensure that empty arrays cannot be entered as query params
self.assertEqual(resp['arangodb_status'], 'connected_authorized')
self.assertTrue(resp['commit_hash'])
self.assertTrue(resp['repo_url'])
resp_json = requests.get(URL + '/').json()
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistent naming for responses -- if it is a response object, call it resp; if it is the deJSONified response body, call it resp_json.

}
)

resp = requests.get(f"{API_URL}/data_sources/{name}")
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repeated test removed

@jayrbolton jayrbolton merged commit 2d2a762 into develop Sep 1, 2020
@jayrbolton jayrbolton deleted the standard_errors branch September 1, 2020 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardise error response structure

2 participants