-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add RAML and integration tests #409
Conversation
Adds raml/ directory with RAML api spec Schemas moved from api/schemas to raml/schemas Add Abao to Travis build for automated integration testing based on API spec Add Abao test hooks to enable some very basic workflow testing
@nagem Thanks for helping me with rebase!! |
This was closed yesterday when a test began failing intermittently. Huge thanks to @ryansanford for helping me track that down, the issue is documented here: APIDevTools/json-schema-ref-parser#26 I ran through 1000 iterations of the tests to make sure there are no more intermittent failures and the workaround is successful |
body: | ||
application/octet-stream: | ||
400: | ||
description: Ticket not for this source IP |
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.
Does RAML allow for multiple instances of the same error code with different descriptions? For example, documenting an endpoint with multiple 4** errors including 400: <some param> is required
as well as 400: <some other param> is not a valid date
.
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.
Or would the standard be to list all error responses in the same description?
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 want to have a particular status code represent only one error condition wherever possible. For instance, I choose to use 400 for un-parseable JSON, then I choose to use 422 to represent json that did not match the schema. 400 would be a valid status code for the second condition, but then the status code becomes ambiguous. In cases where one status code must be used and could represent different situations, the response should contain a body with information describing the specific condition. Thus documenting multiple descriptions for the same error code isn't something we would ever do.
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.
Granted if an existing implementation of an endpoint uses status codes in an ambiguous way, and we aren't changing the status code behavior at that time, documenting the different responses in the same description is the best thing
@nagem I added a commit because userhandler was not actually using user-new.json . I also added input/user-update.json in place of input/user.json That issue might have contributed to the confusion about user schemas. Can you review schemas/input/user-new.json and schemas/input/user-update.json now that I've made this change? |
An overview of the PR LGTM, nice work Colton! |
Adds raml/ directory with RAML api spec Schemas moved from api/schemas to raml/schemas Add Abao to Travis build for automated integration testing based on API spec Add Abao test hooks to enable some very basic workflow testing
@ryansanford Got the docstrings removed for endpoints that exist in RAML |
Keep in mind this pull request and #414 are incompatible, in that megan adds a schema in the old location and I move the schemas folder. Travis will notice this but just be aware that whoever merges second needs to rebase and fix that before merging. |
Closing while I fix up new abao tests for jobs RAML I added |
@gsfr I was able to get CONTRIBUTING.md and TESTING.md added, as well as README.md updated with links to everything |
Closing to rebase after analyses pr merge |
Closing while I add a test for megan's changes to analysis upload |
@nagem I got the test for uploading to /engine with "level=analysis" added to this branch @ryansanford This is ready to merge pending any final review |
Fixes #390
Adds raml/ directory with RAML api spec
Schemas moved from api/schemas to raml/schemas
Add Abao to Travis build for automated integration testing based on API spec
Add Abao test hooks to enable some very basic workflow testing
Ref: #404 , keep making progress on integration tests
Ref: #63 , This issue is not fixed yet but with RAML implemented, it's just a matter of busy work to fill out the rest of the spec.
The only changes to the code in this PR are moving schemas/ directory and adding new-user.json to the list of input schemas (both in config.py). Anything we can do to review / verify those changes is welcome.