Skip to content
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

Merged
merged 49 commits into from
Aug 22, 2016
Merged

Add RAML and integration tests #409

merged 49 commits into from
Aug 22, 2016

Conversation

coltonlw
Copy link
Contributor

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.

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
@coltonlw
Copy link
Contributor Author

@nagem Thanks for helping me with rebase!!

@coltonlw coltonlw closed this Aug 17, 2016
@coltonlw coltonlw deleted the add-raml-testing-squashed branch August 17, 2016 03:04
@coltonlw coltonlw restored the add-raml-testing-squashed branch August 17, 2016 13:19
@coltonlw coltonlw reopened this Aug 17, 2016
@coltonlw
Copy link
Contributor Author

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
Copy link
Contributor

@nagem nagem Aug 17, 2016

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

@coltonlw
Copy link
Contributor Author

@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?

@nagem
Copy link
Contributor

nagem commented Aug 17, 2016

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
@coltonlw
Copy link
Contributor Author

@ryansanford Got the docstrings removed for endpoints that exist in RAML
@gsfr I will work on getting google docs converted to markdown and made publicly accessible

@coltonlw
Copy link
Contributor Author

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.

@coltonlw
Copy link
Contributor Author

Closing while I fix up new abao tests for jobs RAML I added

@coltonlw coltonlw closed this Aug 19, 2016
@coltonlw coltonlw reopened this Aug 20, 2016
@coltonlw
Copy link
Contributor Author

@gsfr I was able to get CONTRIBUTING.md and TESTING.md added, as well as README.md updated with links to everything

@coltonlw
Copy link
Contributor Author

Closing to rebase after analyses pr merge

@coltonlw coltonlw closed this Aug 22, 2016
@coltonlw coltonlw reopened this Aug 22, 2016
@coltonlw
Copy link
Contributor Author

Closing while I add a test for megan's changes to analysis upload

@coltonlw coltonlw closed this Aug 22, 2016
@coltonlw
Copy link
Contributor Author

@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

@coltonlw coltonlw reopened this Aug 22, 2016
@coltonlw coltonlw removed their assignment Aug 22, 2016
@coltonlw coltonlw merged commit 29702e6 into master Aug 22, 2016
@coltonlw coltonlw deleted the add-raml-testing-squashed branch August 22, 2016 20:20
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.

Create prototype API spec and testing process
3 participants