-
Notifications
You must be signed in to change notification settings - Fork 1
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
Features/3 add tests and coverage with GitHub actions #10
Changes from 15 commits
f178677
9a57253
028c377
a1229fb
eaaa04e
12c52a8
d6802d8
085f84a
db2c1de
69e2a51
673fb5d
47781d0
51fd52e
7626c2f
0f61813
8ee4698
f6c379b
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 |
---|---|---|
@@ -0,0 +1,31 @@ | ||
name: Coverage | ||
|
||
on: | ||
workflow_run: | ||
workflows: ["Tests"] | ||
types: | ||
- completed | ||
|
||
jobs: | ||
coverage: | ||
name: Run tests & display coverage | ||
runs-on: ubuntu-latest | ||
if: github.event.workflow_run.event == 'pull_request' && github.event.workflow_run.conclusion == 'success' | ||
permissions: | ||
# Gives the action the necessary permissions for publishing new | ||
# comments in pull requests. | ||
pull-requests: write | ||
# Gives the action the necessary permissions for editing existing | ||
# comments (to avoid publishing multiple comments in the same PR) | ||
contents: write | ||
# Gives the action the necessary permissions for looking up the | ||
# workflow that launched this workflow, and download the related | ||
# artifact that contains the comment to be published | ||
actions: read | ||
steps: | ||
- name: Post comment | ||
uses: py-cov-action/python-coverage-comment-action@v3 | ||
with: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
GITHUB_PR_RUN_ID: ${{ github.event.workflow_run.id }} | ||
verbose: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
name: Tests | ||
|
||
on: | ||
pull_request: | ||
push: | ||
branches: | ||
- "main" | ||
|
||
jobs: | ||
unit-tests: | ||
runs-on: ubuntu-latest | ||
permissions: | ||
# Gives the action the necessary permissions for publishing new | ||
# comments in pull requests. | ||
pull-requests: write | ||
# Gives the action the necessary permissions for pushing data to the | ||
# python-coverage-comment-action branch, and for editing existing | ||
# comments (to avoid publishing multiple comments in the same PR) | ||
contents: write | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- name: Set up Python 3.11 | ||
uses: actions/setup-python@v4 | ||
with: | ||
python-version: 3.11 | ||
- name: Install dependencies | ||
run: | | ||
python -m pip install --upgrade pip | ||
pip install poetry | ||
poetry config virtualenvs.create false | ||
poetry install --no-root | ||
- name: Launch tests & generate report | ||
run: poetry run pytest | ||
- name: Coverage comment | ||
id: coverage_comment | ||
uses: py-cov-action/python-coverage-comment-action@v3 | ||
with: | ||
GITHUB_TOKEN: ${{ github.token }} | ||
verbose: true | ||
- name: Store Pull Request comment to be posted | ||
uses: actions/upload-artifact@v3 | ||
if: steps.coverage_comment.outputs.COMMENT_FILE_WRITTEN == 'true' | ||
with: | ||
# If you use a different name, update COMMENT_ARTIFACT_NAME accordingly | ||
name: python-coverage-comment-action | ||
# If you use a different name, update COMMENT_FILENAME accordingly | ||
path: python-coverage-comment-action.txt |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
from fastapi import FastAPI | ||
from fastapi.responses import RedirectResponse | ||
from fastapi.testclient import TestClient | ||
import starlette.status as status | ||
|
||
|
||
from regtech_api_commons.api.router_wrapper import Router | ||
|
||
router = Router() | ||
app = FastAPI() | ||
|
||
|
||
@router.api_route("/items/{item_id}", methods=["GET"]) | ||
def get_items(item_id: str): | ||
return {"item_id": item_id} | ||
|
||
|
||
def get_not_decorated(item_id: str): | ||
return {"item_id": item_id} | ||
|
||
|
||
async def get_not_decorated_redirect(): | ||
return RedirectResponse("/items-not-decorated/no_item_id", status_code=status.HTTP_302_FOUND) | ||
|
||
|
||
router.add_api_route("/items-not-decorated/{item_id}", get_not_decorated) | ||
router.add_api_route("/items-not-decorated", get_not_decorated_redirect) | ||
|
||
app.include_router(router) | ||
|
||
client = TestClient(app) | ||
|
||
|
||
def test_get_api_route(): | ||
response = client.get("/items/foo") | ||
assert response.status_code == 200, response.text | ||
assert response.json() == {"item_id": "foo"} | ||
|
||
|
||
def test_get_api_route_ends_with_forward_slash(): | ||
response = client.get("/items/foo/") | ||
assert response.status_code == 200, response.text | ||
assert response.json() == {"item_id": "foo"} | ||
|
||
|
||
def test_get_api_route_wrong_path(): | ||
response = client.get("/item/foo/") | ||
assert response.status_code == 404, response.text | ||
assert response.json() != {"item_id": "foo"} | ||
|
||
|
||
def test_get_api_route_not_decorated(): | ||
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. maybe add another test for the not decorated one, so we can see the response being 302 (I think, the redirect one) 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. sorry... should've read the code closer; were you just trying to test out the different ways to hook up a route? I thought you were testing out the different behaviors of router wrapper and fastapi's built-in router. In that case you can remove the redirect test since it's a manually provided redirect. 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. I was just trying to test whether the wrapper hits the same endpoint if a slash is added at the end of the url. I think the not decorated test is not needed here, so removed it. But I can more tests per your suggestion. |
||
response = client.get("/items-not-decorated/foo") | ||
assert response.status_code == 200, response.text | ||
assert response.json() == {"item_id": "foo"} | ||
|
||
|
||
def test_get_api_route_not_decorated_redirect(): | ||
response = client.get("/items-not-decorated") | ||
assert response.history[0].status_code == 302 | ||
assert response.status_code == 200, response.text | ||
assert response.json() == {"item_id": "no_item_id"} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
from regtech_api_commons.models import AuthenticatedUser | ||
|
||
|
||
def test_from_claims(): | ||
test_claims = { | ||
"name": "test", | ||
"preferred_username": "test_user", | ||
"email": "[email protected]", | ||
"sub": "testuser123", | ||
"institutions": ["/TEST1LEI", "/TEST2LEI/TEST2CHILDLEI"], | ||
} | ||
test_authenticated_user = AuthenticatedUser( | ||
claims=test_claims, | ||
name=test_claims.get("name"), | ||
username=test_claims.get("preferred_username"), | ||
email=test_claims.get("email"), | ||
id=test_claims.get("sub"), | ||
institutions=AuthenticatedUser.parse_institutions(test_claims.get("institutions")), | ||
) | ||
|
||
assert AuthenticatedUser.from_claim(test_claims) == test_authenticated_user | ||
|
||
|
||
def test_parse_institutions(): | ||
test_claims_with_institutions = { | ||
"name": "test", | ||
"preferred_username": "test_user", | ||
"email": "[email protected]", | ||
"sub": "testuser123", | ||
"institutions": ["/TEST1LEI", "/TEST2LEI/TEST2CHILDLEI"], | ||
} | ||
assert AuthenticatedUser.from_claim(test_claims_with_institutions).institutions == ["TEST1LEI", "TEST2CHILDLEI"] | ||
|
||
test_claims_without_institutions = { | ||
"name": "test", | ||
"preferred_username": "test_user", | ||
"email": "[email protected]", | ||
"sub": "testuser123", | ||
} | ||
|
||
assert AuthenticatedUser.from_claim(test_claims_without_institutions).institutions == [] | ||
|
||
|
||
def test_is_authenticated(): | ||
test_claims = { | ||
"name": "test", | ||
"preferred_username": "test_user", | ||
"email": "[email protected]", | ||
"sub": "testuser123", | ||
"institutions": ["/TEST1LEI", "/TEST2LEI/TEST2CHILDLEI"], | ||
} | ||
test_authenticated_user = AuthenticatedUser( | ||
claims=test_claims, | ||
name=test_claims.get("name"), | ||
username=test_claims.get("preferred_username"), | ||
email=test_claims.get("email"), | ||
id=test_claims.get("sub"), | ||
institutions=AuthenticatedUser.parse_institutions(test_claims.get("institutions")), | ||
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. why is this one not using the 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. Will look into it after I push the PR for ticket # 62 today. |
||
) | ||
|
||
assert test_authenticated_user.is_authenticated is True |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import pytest | ||
from regtech_api_commons.oauth2.config import KeycloakSettings | ||
|
||
|
||
def test_jwt_opts_valid_values(): | ||
mock_config = { | ||
"jwt_opts_test1": "true", | ||
"jwt_opts_test2": "true", | ||
"jwt_opts_test3": "12", | ||
} | ||
kc_settings = KeycloakSettings(**mock_config) | ||
assert kc_settings.jwt_opts == {"test1": True, "test2": True, "test3": 12} | ||
|
||
|
||
def test_jwt_opts_invalid_values(): | ||
mock_config = { | ||
"jwt_opts_test1": "not a bool or int", | ||
"jwt_opts_test2": "true", | ||
"jwt_opts_test3": "12", | ||
} | ||
with pytest.raises(Exception) as e: | ||
KeycloakSettings(**mock_config) | ||
assert "validation error" in str(e.value) |
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.
Might be good to test the decorated function with both /items/foo and /items/foo/, since the point of the decorator is to make those two urls hit the same endpoint
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.
Makes sense, sure will work on it.