-
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?
Changes from all commits
168bbad
559e0f7
c7c7482
a89bb42
e75b162
4b4fb9c
f3bae2e
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,3 @@ | ||
[submodule "matrix-doc"] | ||
path = matrix-doc | ||
url = https://github.com/matrix-org/matrix-doc.git |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
pytest_plugins = "matrix_spec_coverage_plugin" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
import sys | ||
import re | ||
import yaml | ||
from responses import RequestsMock | ||
|
||
INTERPOLATIONS = [ | ||
("%CLIENT_MAJOR_VERSION%", "r0") | ||
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. As discussed the spec only lists |
||
] | ||
|
||
def interpolate_str(s): | ||
for interpolation in INTERPOLATIONS: | ||
s = s.replace(interpolation[0], interpolation[1]) | ||
return s | ||
|
||
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 commentThe 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. |
||
|
||
MISSING_BASE_PATH = "Not a valid API Base Path: " | ||
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 commentThe 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 |
||
def __init__(self, hostname="http://example.com"): | ||
self.hostname = hostname | ||
self.endpoints = {} | ||
self.called = [] | ||
self.missing = [] | ||
self.total_endpoints = 0 | ||
|
||
def setup_from_files(self, files): | ||
for file in files: | ||
with open(file) as rfile: | ||
definitions = yaml.load(rfile) | ||
base_path = definitions['basePath'] | ||
resolved_base_path = interpolate_str(base_path) | ||
if resolved_base_path not in self.endpoints: | ||
self.endpoints[resolved_base_path] = {} | ||
regex_paths = { endpoint_to_regex(k): v for k,v in definitions['paths'].items() } | ||
self.endpoints[resolved_base_path].update(regex_paths) | ||
endpoints_added = sum(len(v) for v in definitions['paths'].values()) | ||
self.total_endpoints += endpoints_added | ||
|
||
def process_request(self, request): | ||
full_path_url = request.url | ||
method = request.method | ||
body = request.body | ||
for base_path in self.endpoints.keys(): | ||
if base_path in full_path_url: | ||
path_url = full_path_url.replace(base_path, '') | ||
path_url = path_url.replace(self.hostname, '') | ||
break | ||
else: | ||
self.add_called_missing(MISSING_BASE_PATH, request) | ||
return | ||
endpoints = self.endpoints[base_path] | ||
for endpoint in endpoints.keys(): | ||
if re.fullmatch(endpoint, path_url): | ||
break | ||
else: | ||
self.add_called_missing(MISSING_ENDPOINT, request) | ||
return | ||
endpoint_def = endpoints[endpoint] | ||
try: | ||
endpoint_def[method.lower()] | ||
self.add_called(base_path, endpoint, method, body) | ||
except KeyError: | ||
self.add_called_missing(MISSING_METHOD, request) | ||
|
||
|
||
def add_called(self, base_path, endpoint, method, body): | ||
self.called.append((base_path, endpoint, method, body)) | ||
|
||
def add_called_missing(self, error,request): | ||
self.missing.append((error, request.url, request.method, request.body)) | ||
|
||
def print_summary(self): | ||
print("Accessed: %i out of %i endpoints. %0.2f%% Coverage." % | ||
(len(self.called), self.total_endpoints, len(self.called)*100 / self.total_endpoints) | ||
) | ||
if self.missing: | ||
missing_summary = "\n".join(m[0] + ", ".join(m[1:-1]) for m in self.missing) | ||
raise AssertionError("The following invalid API Requests were made:\n" + | ||
missing_summary) | ||
|
||
class RequestsMockWithApiGuide(RequestsMock): | ||
def __init__(self, api_guide, assert_all_requests_are_fired=True): | ||
self.api_guide = api_guide | ||
super().__init__(assert_all_requests_are_fired) | ||
|
||
def _on_request(self, adapter, request, **kwargs): | ||
self.api_guide.process_request(request) | ||
return super()._on_request(adapter, request, **kwargs) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import _pytest | ||
import pytest | ||
from _pytest._pluggy import HookspecMarker | ||
from matrix_spec_coverage import ApiGuide, RequestsMockWithApiGuide | ||
|
||
hookspec = HookspecMarker("pytest") | ||
|
||
# We use this to print api_guide coverage stats | ||
# after pytest has finished running | ||
def pytest_terminal_summary(terminalreporter, exitstatus): | ||
if pytest.responses_with_api_guide: | ||
guide = pytest.responses_with_api_guide.api_guide | ||
guide.print_summary() | ||
|
||
|
||
def build_api_guide(): | ||
import os | ||
from glob import glob | ||
DOC_FOLDER = "../matrix-doc/api/client-server/" | ||
api_files = glob(os.path.join(DOC_FOLDER, '*.yaml')) | ||
if not api_files: | ||
return | ||
guide = ApiGuide() | ||
guide.setup_from_files(API_FILES) | ||
return guide | ||
|
||
# Load api_guide stats into the pytest namespace so | ||
# that we can print a the stats on terminal summary | ||
@hookspec(historic=True) | ||
def pytest_namespace(): | ||
guide = build_api_guide() | ||
return { 'responses_with_api_guide': RequestsMockWithApiGuide(guide) } |
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.