Features/3 add tests and coverage with GitHub actions #10
Conversation
Coverage report
This PR does not seem to contain any modification to coverable code. |
.github/workflows/tests.yml
Outdated
| # comments (to avoid publishing multiple comments in the same PR) | ||
| contents: write | ||
| steps: | ||
| - uses: actions/checkout@v3 |
There was a problem hiding this comment.
There are v4 of the checkout and upload-artifact actions that have library updates and speed improvements. Probably not needed, but trying to keep those updated when I have to modify actions.
There was a problem hiding this comment.
Will update.
pyproject.toml
Outdated
| "INST_DB_USER=user", | ||
| "INST_DB_PWD=user", | ||
| "INST_DB_HOST=localhost:5432", | ||
| "INST_DB_NAME=fi", |
There was a problem hiding this comment.
Shouldn't need the institution DB env vars in this repo.
There was a problem hiding this comment.
I forgot to remove those.
There was a problem hiding this comment.
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.
Makes sense, sure will work on it.
| name = "Test Name" | ||
|
|
||
| mock_get_group = mocker.patch("regtech_api_commons.oauth2.oauth2_admin.OAuth2Admin.get_group") | ||
| mock_get_group.return_value = {"id": lei, "Name": name} |
There was a problem hiding this comment.
I think the goal here would be to patch the KeycloakAdmin get_group_by_path function, similar to how you patched the update_group during the upsert. Othewise, you're bypassing our "get_group" function, which is what we're really trying to test.
|
|
||
| result = oauth2_admin.associate_to_lei(user_id=user_id, lei=lei) | ||
| assert result is None | ||
|
|
There was a problem hiding this comment.
same thing here with test_associate_to_lei and test_associate_to_leis. Looks you're patching the function we're supposed to be testing, versus the KeycloakAdmin functions internal to those functions.
There was a problem hiding this comment.
Thanks @jcadam14 ! I am looking into all your comments and addressing them.
tests/api/test_router_wrapper.py
Outdated
| assert response.json() != {"item_id": "foo"} | ||
|
|
||
|
|
||
| def test_get_api_route_not_decorated(): |
There was a problem hiding this comment.
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.
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.
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.
tests/models/test_auth.py
Outdated
| def test_incorrect_from_claims(): | ||
| test_authenticated_user = AuthenticatedUser( | ||
| claims=test_claims, | ||
| name="test_user_incorrect", | ||
| username="test_incorrect_username", | ||
| email="[email protected]", | ||
| id="test_incorrect_id", | ||
| institutions=["/incorrect_institution_1", "/incorrect_institution_2"], | ||
| ) | ||
|
|
||
| assert AuthenticatedUser.from_claim(test_claims) != test_authenticated_user | ||
|
|
||
|
|
||
| def test_parse_institutions_correct(): | ||
| assert AuthenticatedUser.parse_institutions(test_claims.get("institutions")) == ["TEST1LEI", "TEST2CHILDLEI"] | ||
| assert AuthenticatedUser.parse_institutions(institutions=None) == [] | ||
|
|
||
|
|
||
| def test_parse_institutions_incorrect(): | ||
| assert AuthenticatedUser.parse_institutions(test_claims.get("institutions")) != [ | ||
| "/TESTLEI", | ||
| "/TEST2LEI/TEST2CHILDLEI", | ||
| ] | ||
| assert AuthenticatedUser.parse_institutions(institutions=None) != ["TESTLEI"] |
There was a problem hiding this comment.
the incorrect from claims test isn't needed; but for the institution parsing tests, instead of calling the parse_institutions method directly, pass in the claim with the different scenarios and test the user object's institutions property. i.e. test one with a claim that doesn't have the institutions field.
tests/oauth2/test_oauth2_admin.py
Outdated
| def test_get_claims(): | ||
| token = "Test Token" | ||
|
|
||
| with patch.object(oauth2_admin, "_get_keys", return_value="secret"): |
There was a problem hiding this comment.
instead of mocking the _get_keys, we should mock the requests.get, then you can do that mock.assert_called_with... check
There was a problem hiding this comment.
@lchen-2101 Thanks! Working on it.
tests/models/test_auth.py
Outdated
| 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.
why is this one not using the from_claim?
There was a problem hiding this comment.
Will look into it after I push the PR for ticket # 62 today.
lchen-2101
deleted the
features/3_add_tests_and_coverage_with_github_actions
branch
|
merged, for future reference, when you create the PR, in the comment section during the initial PR creation, you can put |
|
Will do that, thanks! |
[Short description explaining the high-level reason for the pull request]
Additions
-Added tests
-Added coverage.yml and tests.yml
Removals
Changes
Testing
Screenshots
Notes
Todos
Checklist
Testing checklist
Browsers
Accessibility
Other