-
Notifications
You must be signed in to change notification settings - Fork 119
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
Spec Coverage via. matrix-doc in pytest #125
base: master
Are you sure you want to change the base?
Conversation
* uses V2 API Path * add register_as_guest method to MatrixClient Signed-off-by: pik <[email protected]>
Can one of the admins verify this patch? |
MISSING_ENDPOINT = "Not a valid API Endpoint: " | ||
MISSING_METHOD = "Not a valid API Method: " | ||
|
||
class ApiGuide: |
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.
Not a fan of this class-name - but I'd prefer not to have something very long like MatrixApiCoverageStatGatherer
, though maybe that's better than this?
def endpoint_to_regex(s): | ||
# TODO sub by with more specific REGEXes per type | ||
# e.g. roomId, eventId, userId | ||
return re.sub('\{[a-zA-Z]+\}', '[a-zA-Z!\.:-@#]+', s) |
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 is bad, requests, or some other library should provide a way to directly assert whether a request matches e.g. /room_id/{roomId}
I haven't really looked into it, but regexing is not the preferred way for sure.
from responses import RequestsMock | ||
|
||
INTERPOLATIONS = [ | ||
("%CLIENT_MAJOR_VERSION%", "r0") |
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.
As discussed the spec only lists %CLIENT_MAJOR_VERSION%
for all it's docs, potentially this is fail-prone where api/v1
and api/v2
actually differ in Synapse, probably needs to have an issue opened on matrix-doc.
@@ -0,0 +1,3 @@ | |||
[submodule "matrix-doc"] |
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 don't think a git submodule is the right way to approach including the documentation for testing, but I don't know what the "right way" is off the top of my head.
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 don't you like about sub-moduling?
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.
It makes it fairly easy for someone developing to run tests but quite difficult if you've just installed a release version and want to run the tests.
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.
submodules are not fetched automatically you need to ask git to do that, so if you haven't pull matrix-doc it just runs tests without providing you with spec-coverage.
On the other-hand if we move towards using the matrix-doc to also create / respond to mock requests (we don't currently) than indeed you would need matrix-doc to run tests.
Runs with the usual
pytest test
- if matrix-doc submodule is pulled it will attempt to generate coverage stats by parsing the API spec. Requests to paths / methods not provided for in Matrix doc will result in error messages: this currently doesn't supercede testing with mocks but it will raise errors if the mocking does not correspond to spec'd API endpoints (this can be improved later to auto-mock).