Use an app factory pattern to enable app testing #495
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pamelafox
commented
Aug 2, 2023
| @@ -14,14 +14,23 @@ jobs: | |||
| fail-fast: false | |||
| matrix: | |||
| os: ["ubuntu-20.04"] | |||
| python_version: ["3.8", "3.9", "3.10", "3.11"] | |||
| python_version: ["3.9", "3.10", "3.11"] | |||
There was a problem hiding this comment.
Removing 3.8 since we don't currently support it (due to our typing syntax). We can add it back if its an issue for customers.
pamelafox
commented
Aug 2, 2023
| run: | | ||
| cd ./app/frontend | ||
| npm install | ||
| npm run build |
There was a problem hiding this comment.
This must be built since the Flask tests try to hit up index.html
pamelafox
commented
Aug 2, 2023
|
|
||
| @bp.route("/assets/<path:path>") | ||
| def assets(path): | ||
| return send_from_directory("static/assets", path) |
There was a problem hiding this comment.
This is a security upgrade over the previous route that used send_static_file for arbitrary paths.
pamelafox
commented
Aug 2, 2023
| def ask(): | ||
| if not request.json: | ||
| return jsonify({"error": "request must be json"}), 400 | ||
| if not request.is_json: |
There was a problem hiding this comment.
This was a bug, caught by the new tests. Calling request.json causes an immediate 415 HTTP error if theres no JSON passed in, thus it's now request.is_json.
tonybaloney
approved these changes
Aug 3, 2023
|
Great improvement, this will cause a few merge conflicts with existing PRs, but that can't be avoided |
JohananLM
added a commit
to JohananLM/azure-search-openai-demo
that referenced
this pull request
Aug 9, 2023
Use an app factory pattern to enable app testing (Azure-Samples#495)
HughRunyan
pushed a commit
to RMI/RMI_chatbot
that referenced
this pull request
Mar 26, 2024
* Run ruff, add to precommit * Blueprint * Change to blueprint, app factory * Fixing the imports * Build frontend * Update startup * Update startup * Gunicorn command update * Revert yaml change
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
This PR changes app.py to use an app factory pattern (create_app) along with a Flask Blueprint to organize the code. This makes testing much easier and is a general best practice.
This PR is based off the previous PR that ruff-linted the files, so that needs to be merged first to isolate the changes. Then I'll turn the draft into an actual PR.
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
How to Test