Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Add a regression test for using both webclient and client resources simultaneously #11765

Merged
merged 3 commits into from
Jan 20, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11765.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add a unit test that checks both `client` and `webclient` resources will function when simultaneously enabled.
106 changes: 106 additions & 0 deletions tests/http/test_webclient.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
# Copyright 2022 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
from typing import Dict

from twisted.web.resource import Resource

from synapse.app.homeserver import SynapseHomeServer
from synapse.config.server import HttpListenerConfig, HttpResourceConfig, ListenerConfig
from synapse.http.site import SynapseSite

from tests.server import make_request
from tests.unittest import HomeserverTestCase, create_resource_tree, override_config


class WebClientTests(HomeserverTestCase):
@override_config(
{
"web_client_location": "https://example.org",
}
)
def test_webclient_resolves_with_client_resource(self):
"""
Tests that both client and webclient resources can be accessed simultaneously.

This is a regression test created in response to https://github.com/matrix-org/synapse/issues/11763.
"""
for resource_name_order_list in [
["webclient", "client"],
["client", "webclient"],
]:
# Create a dictionary from path regex -> resource
resource_dict: Dict[str, Resource] = {}

for resource_name in resource_name_order_list:
resource_dict.update(
SynapseHomeServer._configure_named_resource(self.hs, resource_name)
)

# Create a root resource which ties the above resources together into one
root_resource = Resource()
create_resource_tree(resource_dict, root_resource)

# Create a site configured with this resource to make HTTP requests against
listener_config = ListenerConfig(
port=8008,
bind_addresses=["127.0.0.1"],
type="http",
http_options=HttpListenerConfig(
resources=[HttpResourceConfig(names=resource_name_order_list)]
),
)
test_site = SynapseSite(
logger_name="synapse.access.http.fake",
site_tag=self.hs.config.server.server_name,
config=listener_config,
resource=root_resource,
server_version_string="1",
max_request_body_size=1234,
reactor=self.reactor,
)

# Attempt to make requests to endpoints on both the webclient and client resources
# on test_site.
self._request_client_and_webclient_resources(test_site)

def _request_client_and_webclient_resources(self, test_site: SynapseSite) -> None:
"""Make a request to an endpoint on both the webclient and client-server resources
of the given SynapseSite.

Args:
test_site: The SynapseSite object to make requests against.
"""

# Ensure that the *webclient* resource is behaving as expected (we get redirected to
# the configured web_client_location)
channel = make_request(
self.reactor,
site=test_site,
method="GET",
path="/_matrix/client",
)
self.assertEqual(channel.code, 302)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have an HTTPStatus.... here? I can never remember what anything other than 200, 400, 404, 500 means. For consistency, ditto for the 200 below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw most tests use integers for status codes (and so does most of the servlet code when specifying the status to respond with). Though a few bits of the code use HTTPStatus, so it looks like we're already inconsistent on this, so 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially did this as I like to use literals in tests where possible (to ensure what we have in the code makes sense). However if these status codes ever change (unlikely) then presumably we'll want to update the tests anyhow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the 302 to an HTTPStatus.FOUND. The status code name is pretty vague, so I added a comment on top as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw most tests use integers for status codes (and so does most of the servlet code when specifying the status to respond with). Though a few bits of the code use HTTPStatus, so it looks like we're already inconsistent on this, so 🤷

I think we've been slowly updating to use HTTPStatus. That seems to be @DMRobertson's plan at least.

Copy link
Contributor

@DMRobertson DMRobertson Jan 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right; maybe I ought to just do it in one big bang if we're going to use them at all.

Being fully candid, I don't have a justification for this except for "I think it's nicer" and a desire to make more use of IntEnum and friends. Happy to drop this if people find it noisey or not useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's nicer and more useful than the status code alone. And if one wants to know the status code, they can simply check the definition.

self.assertEqual(
channel.headers.getRawHeaders("Location"), ["https://example.org"]
)

# Ensure that a request to the *client* resource works.
channel = make_request(
self.reactor,
site=test_site,
method="GET",
path="/_matrix/client/v3/login",
)
self.assertEqual(channel.code, 200)
self.assertIn("flows", channel.json_body)