Add test for ProtectedResourceMetadataParsing #1236
Conversation
There was a problem hiding this comment.
This PR is not testing the fact that the new fields are supported. It's testing that the class is properly validated, which it seems we are testing Pydantic here. The same can be said about the tests above.
I think an ideal test would be to use the new added literal fields in an auth flow. But that's an issue that comes from before, because we actually don't have tests for it...
Anyway, I'm okay with either merging or closing this.
tests/shared/test_auth.py
Outdated
| @@ -37,3 +38,84 @@ def test_oidc(self): | |||
| "userinfo_endpoint": "https://example.com/oauth2/userInfo", | |||
| } | |||
| ) | |||
|
|
|||
|
|
|||
| class TestProtectedResourceMetadataInvalid: | |||
There was a problem hiding this comment.
We need a PR to drop those classes. They are following a very old Python testing layout.
You can ignore this comment.
Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
Kludex
dismissed
their stale review
Actually, I don't think this is the way to go.
…resource checking
|
@Kludex tests updated |
|
@Kludex done with the requested changes |
| metadata = response.json() | ||
| assert metadata == snapshot( |
There was a problem hiding this comment.
| metadata = response.json() | |
| assert metadata == snapshot( | |
| assert response.json() == snapshot( |
tests/shared/test_auth.py
Outdated
There was a problem hiding this comment.
Are the tests you added in here necessary?
There was a problem hiding this comment.
To me it prevents someone to introduce regression in the model description by checking against a realistic payload
There was a problem hiding this comment.
I don't think it will have that effect... People that modify the model class will just update the test... The assertion is just making sure the validation passes, there's no assertion that makes use of any further logic.
|
Maybe we could benefit more if we move the test you added to a new file, so we can actually don't mix bad practices with good ones. |
|
I will create a separated file, actually could be directly in the test/server/auth/test_proctected_resource.py |
tests/shared/test_auth.py
Outdated
There was a problem hiding this comment.
I don't think it will have that effect... People that modify the model class will just update the test... The assertion is just making sure the validation passes, there's no assertion that makes use of any further logic.
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
|
As per your advice, I removed the test that is just on the model, and fixed naming. |
I don't think you get anything from it, it's more often seen in old test suites and when using I prefer to stick with what modern packages do: not have wrapper classes. This is my opinion tho. |
Motivation and Context
Add tests cases for insurring ProtectedResourceMetadata validation is correctly done
How Has This Been Tested?
with pytest
Types of changes
Checklist