-
Notifications
You must be signed in to change notification settings - Fork 384
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
Change OpenAPI backend generator config #4376
base: main
Are you sure you want to change the base?
Changes from 1 commit
b3868fc
ee54063
f7ef3fe
573e5d0
4d5c26f
0081cc2
5fff8df
ee6eed7
6821c2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,44 +13,34 @@ | |
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
from datetime import datetime | ||
import json | ||
import logging | ||
import os | ||
import re | ||
from datetime import datetime | ||
from typing import Any, NoReturn, Optional | ||
|
||
import flask | ||
import flask.views | ||
import werkzeug.exceptions | ||
|
||
import google.appengine.api | ||
import werkzeug.exceptions | ||
from flask import render_template, session | ||
from flask_cors import CORS | ||
from google.cloud import ndb # type: ignore | ||
|
||
import settings | ||
from api import api_specs | ||
from framework import csp | ||
from framework import permissions | ||
from framework import secrets | ||
from framework import users | ||
from framework import utils | ||
from framework import xsrf | ||
from internals import approval_defs | ||
from internals import notifier_helpers | ||
from internals import user_models | ||
from framework import csp, permissions, secrets, users, utils, xsrf | ||
from internals import approval_defs, notifier_helpers, user_models | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for updating the imports like this? This does not hurt the functionality, but it is not the typical import structure used for the project There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems like it runs |
||
from internals.core_enums import ( | ||
ALL_ORIGIN_TRIAL_STAGE_TYPES, | ||
OT_ACTIVATION_FAILED, | ||
OT_CREATION_FAILED, | ||
OT_READY_FOR_CREATION) | ||
OT_READY_FOR_CREATION, | ||
) | ||
from internals.core_models import FeatureEntry, MilestoneSet, Stage | ||
from internals.data_types import CHANGED_FIELDS_LIST_TYPE | ||
|
||
from flask import session | ||
from flask import render_template | ||
from flask_cors import CORS | ||
from gen.py.chromestatus_openapi.chromestatus_openapi.models.base_model import Model | ||
|
||
# Our API responses are prefixed with this ro prevent attacks that | ||
# exploit <script src="...">. See go/xssi. | ||
XSSI_PREFIX = ')]}\'\n' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,6 @@ | |
"prepare": "npm run build" | ||
}, | ||
"devDependencies": { | ||
"typescript": "^5.6" | ||
"typescript": "^4.0 || ^5.0" | ||
} | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
# NOTE: This file is auto generated by OpenAPI Generator. | ||
# URL: https://openapi-generator.tech | ||
# | ||
# ref: https://docs.github.com/en/actions/automating-builds-and-tests/building-and-testing-python | ||
|
||
name: chromestatus_openapi Python package | ||
|
||
on: [push, pull_request] | ||
|
||
jobs: | ||
build: | ||
|
||
runs-on: ubuntu-latest | ||
strategy: | ||
matrix: | ||
python-version: ["3.7", "3.8", "3.9", "3.10", "3.11"] | ||
|
||
steps: | ||
- uses: actions/checkout@v3 | ||
- name: Set up Python ${{ matrix.python-version }} | ||
uses: actions/setup-python@v4 | ||
with: | ||
python-version: ${{ matrix.python-version }} | ||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install flake8 pytest | ||
if [ -f requirements.txt ]; then pip install -r requirements.txt; fi | ||
if [ -f test-requirements.txt ]; then pip install -r test-requirements.txt; fi | ||
- name: Lint with flake8 | ||
run: | | ||
# stop the build if there are Python syntax errors or undefined names | ||
flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics | ||
# exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide | ||
flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics | ||
- name: Test with pytest | ||
run: | | ||
pytest |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
# NOTE: This file is auto generated by OpenAPI Generator. | ||
# URL: https://openapi-generator.tech | ||
# | ||
# ref: https://docs.gitlab.com/ee/ci/README.html | ||
# ref: https://gitlab.com/gitlab-org/gitlab/-/blob/master/lib/gitlab/ci/templates/Python.gitlab-ci.yml | ||
|
||
stages: | ||
- test | ||
|
||
.pytest: | ||
stage: test | ||
script: | ||
- pip install -r requirements.txt | ||
- pip install -r test-requirements.txt | ||
- pytest --cov=chromestatus_openapi | ||
|
||
pytest-3.7: | ||
extends: .pytest | ||
image: python:3.7-alpine | ||
pytest-3.8: | ||
extends: .pytest | ||
image: python:3.8-alpine | ||
pytest-3.9: | ||
extends: .pytest | ||
image: python:3.9-alpine | ||
pytest-3.10: | ||
extends: .pytest | ||
image: python:3.10-alpine | ||
pytest-3.11: | ||
extends: .pytest | ||
image: python:3.11-alpine |
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.
model_construct takes in the expected object type for each argument. (votes should expect a variable of type
List[Votes]
). However, this is a special case where the existing code does this special conversion to dictionaries first. So instead, you can use the class method from_dict.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.
This did not work either. This indeed helps convert the dict to model and then using to_dict() to convert it back, but it seems like the serialization & deserialization during the process changed the order of stuff we put in the dict and therefore differ from the test case. How would you suggest circumventing this situation?
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.
What error are you seeing? When I tried this suggestion earlier, the test started passing.
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.
i got
FAIL: test_do_get__success (api.reviews_api_test.GatesAPITest.test_do_get__success) Handler retrieves all gates associated with a given feature. Traceback (most recent call last): File "/opt/homebrew/Cellar/python@3.11/3.11.8/Frameworks/Python.framework/Versions/3.11/lib/python3.11/unittest/mock.py", line 1375, in patched return func(*newargs, **newkeywargs) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/mark/chromium-dashboard/api/reviews_api_test.py", line 349, in test_do_get__success self.assertEqual(actual, expected) AssertionError: {'gat[126 chars]e', 'state': 1, 'assignee_emails': [], 'additi[125 chars]ne}]} != {'gat[126 chars]e', 'escalation_email': None, 'state': 1, 'req[265 chars]']}]} Diff is 674 characters long. Set self.maxDiff to None to see it.
Looks like the order is still messed up. The only change I made was to undo the changes in the current commit toreturn GetVotesResponse.from_dict({'votes': dicts}).to_dict()
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.
That failing test is for a different method than this one. This test should be passing now. Could you verify that?
The GatesAPITest.test_do_get__success test is for another class in the same file. I verified that GatesAPITest.test_do_get__success is failing for me too.
You will see that it is not an order issue but it is removing the null values instead of defaulting to None. That would be the first thing I would look into.
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.
Seems like if you need to add
"nullable: true"
to those fields I commented out in my above comment (similar to escalation_email). Then, it will work. Then you can revert the commented lines I made in the test.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.
pasted the wrong test.. my bad. Adding
nullable: true
gets the test passed. I'll modify other specs as well to see if all tests could pass. Thanks!