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.

Sign up for GitHub

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

Jump to bottom

tests: add OasstApiClient contract tests #216

Merged
merged 12 commits into from Jan 1, 2023
Merged

tests: add OasstApiClient contract tests #216

merged 12 commits into from Jan 1, 2023

Conversation

jack-michaud
Copy link
Collaborator

@jack-michaud jack-michaud commented Dec 31, 2022 •
edited

Fixes #205

  • Adds print_openapi_schema to backend
  • Add start-mock-server.sh backend script
  • Add contract tests for OasstApiClient (with test script)
    • Contract tests will automatically catch changes to the backend OpenAPI schema.
  • Add some additional type information and response model annotations to fill out the empty OpenAPI schema

Not in this PR but will complete in my next PR: adding a github workflow which automatically runs the tests for each PR and push.

@fozziethebeat fozziethebeat added the testing all things testing label Jan 1, 2023
@jack-michaud jack-michaud changed the title chore: add print_openapi_schema and start-mock-server.sh script tests: add OasstApiClient contract tests Jan 1, 2023
@jack-michaud jack-michaud marked this pull request as ready for review January 1, 2023 15:24
andreaskoepf
andreaskoepf reviewed Jan 1, 2023
View reviewed changes
backend/oasst_backend/api/v1/tasks.py
@@ -204,7 +204,7 @@ def tasks_acknowledge_failure(
raise OasstError("Failed to not acknowledge task.", OasstErrorCode.TASK_NACK_FAILED)


@router.post("/interaction")
@router.post("/interaction", response_model=protocol_schema.TaskDone)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yk note on how to specify multiple possible response_models: https://stackoverflow.com/questions/72919006/is-there-any-way-to-have-multiple-response-models-in-fastapi-openapi

andreaskoepf
andreaskoepf approved these changes Jan 1, 2023
View reviewed changes
Copy link
Collaborator

@andreaskoepf andreaskoepf left a comment

Choose a reason for hiding this comment

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

Remaining question about the best location for the print-api call .. but overall approved.

discord-bot/bot/api_client.py Show resolved Hide resolved
backend/print_openapi_schema.py Outdated Show resolved Hide resolved
discord-bot/bot/api_client.py Show resolved Hide resolved
@andreaskoepf andreaskoepf mentioned this pull request Jan 1, 2023
Closed
jack-michaud
jack-michaud commented Jan 1, 2023
View reviewed changes
discord-bot/bot/api_client.py Show resolved Hide resolved
discord-bot/tests/test_oasst_api_client.py Show resolved Hide resolved
backend/print_openapi_schema.py Outdated Show resolved Hide resolved
@andreaskoepf andreaskoepf enabled auto-merge (rebase) January 1, 2023 17:56
@andreaskoepf andreaskoepf merged commit 29b0887 into LAION-AI:main Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreaskoepf andreaskoepf andreaskoepf approved these changes

@yk yk Awaiting requested review from yk yk is a code owner

Assignees
No one assigned
Labels
testing all things testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Contract tests between API and Discord Bot
3 participants
@jack-michaud @andreaskoepf @fozziethebeat