First HTTP functional test of the RPC interface

Message ID 20200412165546.GA2595450@tsubame.mg0.fr
State New
Headers show
Series
  • First HTTP functional test of the RPC interface
Related show

Commit Message

Frédéric Mangano-Tarumi April 12, 2020, 4:55 p.m. UTC
Though barely useful in its current state, it shows how to integrate
pytest with SQLAlchemy and Werkzeug, providing a test framework for the
potential future Flask port, while actually testing the current PHP
implementation.
---
 aurweb/test/__init__.py |  0
 aurweb/test/conftest.py | 51 +++++++++++++++++++++++++++++++++++++++++
 aurweb/test/test_rpc.py | 21 +++++++++++++++++
 aurweb/test/wsgihttp.py | 38 ++++++++++++++++++++++++++++++
 test/README.md          |  3 +++
 test/rpc.t              |  2 ++
 6 files changed, 115 insertions(+)
 create mode 100644 aurweb/test/__init__.py
 create mode 100644 aurweb/test/conftest.py
 create mode 100644 aurweb/test/test_rpc.py
 create mode 100644 aurweb/test/wsgihttp.py
 create mode 100755 test/rpc.t

Comments

Lukas Fleischer April 14, 2020, 12:54 p.m. UTC | #1
Hi Frédéric,

Thanks for the work! My first thoughts below.

On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> Though barely useful in its current state, it shows how to integrate
> pytest with SQLAlchemy and Werkzeug, providing a test framework for the
> potential future Flask port, while actually testing the current PHP
> implementation.
> ---
>  aurweb/test/__init__.py |  0
>  aurweb/test/conftest.py | 51 +++++++++++++++++++++++++++++++++++++++++
>  aurweb/test/test_rpc.py | 21 +++++++++++++++++
>  aurweb/test/wsgihttp.py | 38 ++++++++++++++++++++++++++++++
>  test/README.md          |  3 +++
>  test/rpc.t              |  2 ++
>  6 files changed, 115 insertions(+)
>  create mode 100644 aurweb/test/__init__.py
>  create mode 100644 aurweb/test/conftest.py
>  create mode 100644 aurweb/test/test_rpc.py
>  create mode 100644 aurweb/test/wsgihttp.py
>  create mode 100755 test/rpc.t
> [...]
> +@pytest.fixture
> +def client():
> +    """
> +    Build a Werkzeug test client for making HTTP requests to AUR. It requires
> +    that the AUR test website is already running at `[options] aur_location`,
> +    specified in the configuration file.
> +
> +    When aurweb becomes a pure Flask application, this should return Flask\u2019s
> +    test_client(), which is a Werkzeug test client too.
> +    https://flask.palletsprojects.com/en/1.1.x/testing/#the-testing-skeleton
> +    """
> +    base_uri = aurweb.config.get("options", "aur_location")
> +    proxy = WsgiHttpProxy(base_uri)
> +    return werkzeug.test.Client(proxy, Response)

If I understand correctly, this runs tests against our production setup
at aur.archlinux.org. Is that what you ultimately plan to do in the
transition phase?

On a related note, the current test suite has been designed to run in an
isolated environment without any prior configuration or network
connectivity. Do we want to break either of these assumptions? Breaking
the latter might be okay, but I would like to keep the test suite
self-contained. If that turns out to be too hard, I'd be fine with
splitting the tests into two parts (a self-contained part and another
part that requires a local aurweb setup). We also need to think about
how to integrate all that with CI.

Lukas
Frédéric Mangano-Tarumi April 14, 2020, 1:26 p.m. UTC | #2
Hi Lukas,

Lukas Fleischer [2020-04-14 08:54:03 -0400]
> On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> > +    base_uri = aurweb.config.get("options", "aur_location")
> > +    proxy = WsgiHttpProxy(base_uri)
> > +    return werkzeug.test.Client(proxy, Response)
> 
> If I understand correctly, this runs tests against our production setup
> at aur.archlinux.org. Is that what you ultimately plan to do in the
> transition phase?

No way! aur_location refers to the development setup address. Its
default value in our sample configuration is http://localhost:8080,
which is fine. Note that in my first test I also accessed the database
directly too, so the environment is not entirely blackboxed.

> On a related note, the current test suite has been designed to run in an
> isolated environment without any prior configuration or network
> connectivity.

We could spawn PHP and prepare the SQLite database when the test suite
starts, then clean up everything on exit. If we use Unix sockets
everywhere, we may be able to avoid using a TCP port too.

That is only useful during the transition phase, and as long as we don’t
have Docker. If you believe we should invest time in auto-spawning, I
can submit a follow-up patch. Beside self-containment, the test suite
will have full control over its data set, so it actually sounds like a
good thing to do even in that temporary phase. It shouldn’t be too hard
anyway. What do you think?
Lukas Fleischer April 14, 2020, 1:36 p.m. UTC | #3
On Tue, 14 Apr 2020 at 09:26:44, Frédéric Mangano-Tarumi wrote:
> Hi Lukas,
> 
> Lukas Fleischer [2020-04-14 08:54:03 -0400]
> > On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> > > +    base_uri = aurweb.config.get("options", "aur_location")
> > > +    proxy = WsgiHttpProxy(base_uri)
> > > +    return werkzeug.test.Client(proxy, Response)
> > 
> > If I understand correctly, this runs tests against our production setup
> > at aur.archlinux.org. Is that what you ultimately plan to do in the
> > transition phase?
> 
> No way! aur_location refers to the development setup address. Its
> default value in our sample configuration is http://localhost:8080,
> which is fine. Note that in my first test I also accessed the database
> directly too, so the environment is not entirely blackboxed.

Could you quickly point me to where that default value is configured?
Both the testing configuration generated by test/setup.sh and the sample
configuration at conf/config.defaults seem to use our production setup.

> > On a related note, the current test suite has been designed to run in an
> > isolated environment without any prior configuration or network
> > connectivity.
> 
> We could spawn PHP and prepare the SQLite database when the test suite
> starts, then clean up everything on exit. If we use Unix sockets
> everywhere, we may be able to avoid using a TCP port too.
> 
> That is only useful during the transition phase, and as long as we don\u2019t
> have Docker. If you believe we should invest time in auto-spawning, I
> can submit a follow-up patch. Beside self-containment, the test suite
> will have full control over its data set, so it actually sounds like a
> good thing to do even in that temporary phase. It shouldn\u2019t be too hard
> anyway. What do you think?

If it's easy to do that without a huge overhead, I am all for it.

We should still try to keep the tests that require these preparatory
steps separate from tests that don't, just in case anybody wants to run
the tests without having to install all the additional components.

Lukas
Frédéric Mangano-Tarumi April 14, 2020, 1:49 p.m. UTC | #4
Lukas Fleischer [2020-04-14 09:36:11 -0400]
> On Tue, 14 Apr 2020 at 09:26:44, Frédéric Mangano-Tarumi wrote:
> > No way! aur_location refers to the development setup address. Its
> > default value in our sample configuration is http://localhost:8080,
> > which is fine. Note that in my first test I also accessed the database
> > directly too, so the environment is not entirely blackboxed.
> 
> Could you quickly point me to where that default value is configured?
> Both the testing configuration generated by test/setup.sh and the sample
> configuration at conf/config.defaults seem to use our production setup.

My bad, you’re right. I had changed it following the TESTING
instructions. aur_location is clearly the variable to use, but in the
current state some people will miss it and the test suite would send
requests to production. One more argument in favor of the self-contained
test suite.

> We should still try to keep the tests that require these preparatory
> steps separate from tests that don't, just in case anybody wants to run
> the tests without having to install all the additional components.

Easy enough. We can spawn the server on the first instantiation of the
test client fixture. We could add some more detection steps to mark the
tests as skipped if spawning fails because of missing dependencies.
Lukas Fleischer April 15, 2020, 12:08 p.m. UTC | #5
On Tue, 14 Apr 2020 at 09:49:06, Frédéric Mangano-Tarumi wrote:
> My bad, you\u2019re right. I had changed it following the TESTING
> instructions. aur_location is clearly the variable to use, but in the
> current state some people will miss it and the test suite would send
> requests to production. One more argument in favor of the self-contained
> test suite.

Our test suite already uses a separate configuration which is
auto-generated in test/setup.sh, so it is already self-contained in that
sense; we only need to update aur_location there.

> > We should still try to keep the tests that require these preparatory
> > steps separate from tests that don't, just in case anybody wants to run
> > the tests without having to install all the additional components.
> 
> Easy enough. We can spawn the server on the first instantiation of the
> test client fixture. We could add some more detection steps to mark the
> tests as skipped if spawning fails because of missing dependencies.

Great! I would probably prefer having a simple way of telling the test
suite to run with or without these tests, though. We should not let the
test suite pass if we intended to run all tests but forgot to install
some dependency. Maybe we can use an environment variable for that?

Lukas
Frédéric Mangano-Tarumi April 15, 2020, 12:44 p.m. UTC | #6
Lukas Fleischer [2020-04-15 08:08:08 -0400]
> Our test suite already uses a separate configuration which is
> auto-generated in test/setup.sh, so it is already self-contained in that
> sense; we only need to update aur_location there.

I’m hesitant about using setup.sh because it does more than we need. It
aims at being universal but that means it prepares an environment
suitable for all tests, but run for every test. That’s quite a waste of
resources. If it’s just for the configuration file, we could make a
config.test.defaults and make setup.sh use it except for the few
variables it needs to override.

> Great! I would probably prefer having a simple way of telling the test
> suite to run with or without these tests, though. We should not let the
> test suite pass if we intended to run all tests but forgot to install
> some dependency. Maybe we can use an environment variable for that?

In Perl there’s a convention to have `t/` for regular tests, and `xt/`
(extended tests I guess) for developers. We could follow a similar
convention and have `test_http` or something. The downside is that
unless they are included by default, developers might have a tendency to
forget running them. We could create `test/regular/` and `test/http/` to
give developers an easy way to exclude HTTP tests, while still running
them by default.

Still, I wonder if a skip is that bad an option. It’s true it’s not as
explicit as a fail, but a skipped test is a common convention for the
test suite to indicate it’s missing something, isn’t it?
Lukas Fleischer April 15, 2020, 9:06 p.m. UTC | #7
On Wed, 15 Apr 2020 at 08:44:44, Frédéric Mangano-Tarumi wrote:
> Lukas Fleischer [2020-04-15 08:08:08 -0400]
> > Our test suite already uses a separate configuration which is
> > auto-generated in test/setup.sh, so it is already self-contained in that
> > sense; we only need to update aur_location there.
> 
> I\u2019m hesitant about using setup.sh because it does more than we need. It
> aims at being universal but that means it prepares an environment
> suitable for all tests, but run for every test. That\u2019s quite a waste of
> resources. If it\u2019s just for the configuration file, we could make a
> config.test.defaults and make setup.sh use it except for the few
> variables it needs to override.

The new tests are going to require mock data in the database, too,
right? Do you intend to populate the database separately for each test?
Note that many tests require a common basic set of data, such as user
accounts and packages. If we want different initialization procedures
for each test, we should at least have some way of including this common
data set.

The setup script only takes a couple of milliseconds, so having to
execute it multiple times doesn't bother me too much. If you have any
suggestions on how to reduce some of the duplicate steps, feel free to
post them here, though.

> Still, I wonder if a skip is that bad an option. It\u2019s true it\u2019s not as
> explicit as a fail, but a skipped test is a common convention for the
> test suite to indicate it\u2019s missing something, isn\u2019t it?

Yeah, I guess you are right, especially since there probably is an
option to make skipped tests fail the test suite.

Lukas
Frédéric Mangano-Tarumi April 15, 2020, 9:54 p.m. UTC | #8
Lukas Fleischer [2020-04-15 17:06:15 -0400]
> The new tests are going to require mock data in the database, too,
> right? Do you intend to populate the database separately for each test?
> Note that many tests require a common basic set of data, such as user
> accounts and packages. If we want different initialization procedures
> for each test, we should at least have some way of including this common
> data set.

We have the whole Python arsenal within arm’s reach. I’m sure we’ll find
a way. It’s dumb code anyway.

> The setup script only takes a couple of milliseconds, so having to
> execute it multiple times doesn't bother me too much. If you have any
> suggestions on how to reduce some of the duplicate steps, feel free to
> post them here, though.

When I run test/t2600-rendercomment.t and measure the time, it spends
almost 8 seconds on setup.sh, then 3 seconds to actually run the tests.
That’s quite significant.

$ ./t2600-rendercomment.t | ts -s %.s
0.000010 Starting setup.sh
7.887877 Compeleted setup.sh
8.540758 ok 1 - Test comment rendering.
9.050442 ok 2 - Test Markdown conversion.
9.574958 ok 3 - Test HTML sanitizing.
10.098891 ok 4 - Test link conversion.
10.617952 ok 5 - Test Git commit linkification.
11.156466 ok 6 - Test Flyspray issue linkification.
11.657045 ok 7 - Test headings lowering.
11.673559 # passed all 7 test(s)
11.673628 1..7
Lukas Fleischer April 16, 2020, 8:58 p.m. UTC | #9
On Wed, 15 Apr 2020 at 17:54:45, Frédéric Mangano-Tarumi wrote:
> When I run test/t2600-rendercomment.t and measure the time, it spends
> almost 8 seconds on setup.sh, then 3 seconds to actually run the tests.
> That\u2019s quite significant.
> 
> $ ./t2600-rendercomment.t | ts -s %.s
> 0.000010 Starting setup.sh
> 7.887877 Compeleted setup.sh
> 8.540758 ok 1 - Test comment rendering.
> 9.050442 ok 2 - Test Markdown conversion.
> 9.574958 ok 3 - Test HTML sanitizing.
> 10.098891 ok 4 - Test link conversion.
> 10.617952 ok 5 - Test Git commit linkification.
> 11.156466 ok 6 - Test Flyspray issue linkification.
> 11.657045 ok 7 - Test headings lowering.
> 11.673559 # passed all 7 test(s)
> 11.673628 1..7

Interesting, I get very different results here. Are you using a
traditional HDD? Since setup.sh does a lot of disk I/O, I could imagine
a significant slowdown with physical disks. Patches are always welcome!
Filipe Laíns April 16, 2020, 9:58 p.m. UTC | #10
On Sun, 2020-04-12 at 18:55 +0200, Frédéric Mangano-Tarumi wrote:
> Though barely useful in its current state, it shows how to integrate
> pytest with SQLAlchemy and Werkzeug, providing a test framework for the
> potential future Flask port, while actually testing the current PHP
> implementation.
> ---
>  aurweb/test/__init__.py |  0
>  aurweb/test/conftest.py | 51 +++++++++++++++++++++++++++++++++++++++++
>  aurweb/test/test_rpc.py | 21 +++++++++++++++++
>  aurweb/test/wsgihttp.py | 38 ++++++++++++++++++++++++++++++
>  test/README.md          |  3 +++
>  test/rpc.t              |  2 ++
>  6 files changed, 115 insertions(+)
>  create mode 100644 aurweb/test/__init__.py
>  create mode 100644 aurweb/test/conftest.py
>  create mode 100644 aurweb/test/test_rpc.py
>  create mode 100644 aurweb/test/wsgihttp.py
>  create mode 100755 test/rpc.t
> 
> diff --git a/aurweb/test/__init__.py b/aurweb/test/__init__.py
> new file mode 100644
> index 00000000..e69de29b
> diff --git a/aurweb/test/conftest.py b/aurweb/test/conftest.py
> new file mode 100644
> index 00000000..49cc2f6e
> --- /dev/null
> +++ b/aurweb/test/conftest.py
> @@ -0,0 +1,51 @@
> +"""
> +Fixtures for pytest.
> +
> +This module is automatically loaded by pytest.
> +"""
> +
> +import pytest
> +import sqlalchemy
> +import werkzeug.test
> +import werkzeug.wrappers
> +import werkzeug.wrappers.json
> +
> +import aurweb.config
> +import aurweb.db
> +from aurweb.test.wsgihttp import WsgiHttpProxy
> +
> +
> +class Response(werkzeug.wrappers.CommonResponseDescriptorsMixin,
> +               werkzeug.wrappers.json.JSONMixin,
> +               werkzeug.wrappers.BaseResponse):
> +    """
> +    Custom response object to be returned by the test client. More mixins could
> +    be added if need be.
> +
> +    See https://werkzeug.palletsprojects.com/en/1.0.x/wrappers/#mixin-classes
> +    """
> +    pass
> +
> +
> +@pytest.fixture
> +def client():
> +    """
> +    Build a Werkzeug test client for making HTTP requests to AUR. It requires
> +    that the AUR test website is already running at `[options] aur_location`,
> +    specified in the configuration file.
> +
> +    When aurweb becomes a pure Flask application, this should return Flask’s
> +    test_client(), which is a Werkzeug test client too.
> +    https://flask.palletsprojects.com/en/1.1.x/testing/#the-testing-skeleton
> +    """
> +    base_uri = aurweb.config.get("options", "aur_location")
> +    proxy = WsgiHttpProxy(base_uri)
> +    return werkzeug.test.Client(proxy, Response)
> +
> +
> +@pytest.fixture(scope="session")
> +def db_engine():
> +    """
> +    Return an SQLAlchemy engine to the configured database.
> +    """
> +    return sqlalchemy.create_engine(aurweb.db.get_sqlalchemy_url())
> diff --git a/aurweb/test/test_rpc.py b/aurweb/test/test_rpc.py
> new file mode 100644
> index 00000000..7079145c
> --- /dev/null
> +++ b/aurweb/test/test_rpc.py
> @@ -0,0 +1,21 @@
> +"""
> +Test suite for the RPC interface.
> +
> +See also `doc/rpc.txt` for the RPC interface documentation.
> +"""
> +
> +import pytest
> +from sqlalchemy.sql import select
> +
> +from aurweb.schema import Packages
> +
> +
> +def test_search_by_name(client, db_engine):
> +    """Take a package from the database, and find it through the RPC interface."""
> +    with db_engine.connect() as conn:
> +        pkg = conn.execute(select([Packages]).limit(1)).fetchone()
> +        if pkg is None:
> +            pytest.skip("needs at least one package in the database")
> +    resp = client.get("/rpc/", query_string={"v": "5", "type": "search", "arg": pkg["Name"]})
> +    result = resp.json
> +    assert result["resultcount"] >= 1
> diff --git a/aurweb/test/wsgihttp.py b/aurweb/test/wsgihttp.py
> new file mode 100644
> index 00000000..5b9d8040
> --- /dev/null
> +++ b/aurweb/test/wsgihttp.py
> @@ -0,0 +1,38 @@
> +import http.client
> +import urllib.parse
> +
> +
> +class WsgiHttpProxy:
> +    """
> +    WSGI-to-HTTP proxy, that is to say a WSGI application that forwards every
> +    WSGI request to an HTTP server, then the HTTP response back to WSGI.
> +
> +    The base URL the constructor takes is something like
> +    `http://localhost:8080`. It must not contain a path, a query string or a
> +    fragment, as the proxy wouldn’t now what to do with it.
> +
> +    Only the HTTP scheme is supported, but HTTPS could probably be easily added.
> +    """
> +
> +    def __init__(self, base_url):
> +        parts = urllib.parse.urlsplit(base_url)
> +        self.netloc = parts.netloc
> +        # Limitations of this dumb proxy
> +        assert parts.scheme == "http"
> +        assert parts.path in ("", "/")

Why a tuple?

> +        assert parts.query == ""
> +        assert parts.fragment == ""
> +
> +    def __call__(self, environ, start_response):
> +        conn = http.client.HTTPConnection(self.netloc)
> +        conn.request(
> +            method=environ["REQUEST_METHOD"],
> +            url=urllib.parse.urlunsplit((
> +                "http", self.netloc,
> +                urllib.parse.quote(environ["PATH_INFO"]),
> +                environ["QUERY_STRING"], "")),
> +            body=environ["wsgi.input"].read(int(environ.get("CONTENT_LENGTH", 0))),
> +        )
> +        resp = conn.getresponse()
> +        start_response(f"{resp.status} {resp.reason}", resp.getheaders())
> +        return resp
> diff --git a/test/README.md b/test/README.md
> index de7eff18..cc8baf33 100644
> --- a/test/README.md
> +++ b/test/README.md
> @@ -20,8 +20,11 @@ For all the test to run, the following Arch packages should be installed:
>  - python-bleach
>  - python-markdown
>  - python-pygit2
> +- python-pytest
> +- python-pytest-tap
>  - python-sqlalchemy
>  - python-srcinfo
> +- python-werkzeug
>  
>  Writing tests
>  -------------
> diff --git a/test/rpc.t b/test/rpc.t
> new file mode 100755
> index 00000000..f950f7df
> --- /dev/null
> +++ b/test/rpc.t
> @@ -0,0 +1,2 @@
> +#!/bin/sh
> +pytest --tap-stream "$(dirname "$0")/../aurweb/test/test_rpc.py"

I think we should be using pytest-runner here. What do you think Lukas?

Otherwise LGTM.

Cheers,
Filipe Laíns
Frédéric Mangano-Tarumi April 17, 2020, 8:20 p.m. UTC | #11
Lukas Fleischer [2020-04-16 16:58:51 -0400]
> On Wed, 15 Apr 2020 at 17:54:45, Frédéric Mangano-Tarumi wrote:
> > When I run test/t2600-rendercomment.t and measure the time, it spends
> > almost 8 seconds on setup.sh, then 3 seconds to actually run the tests.
> > That\u2019s quite significant.
> > 
> > $ ./t2600-rendercomment.t | ts -s %.s
> > 0.000010 Starting setup.sh
> > 7.887877 Compeleted setup.sh
> > 8.540758 ok 1 - Test comment rendering.
> > 9.050442 ok 2 - Test Markdown conversion.
> > 9.574958 ok 3 - Test HTML sanitizing.
> > 10.098891 ok 4 - Test link conversion.
> > 10.617952 ok 5 - Test Git commit linkification.
> > 11.156466 ok 6 - Test Flyspray issue linkification.
> > 11.657045 ok 7 - Test headings lowering.
> > 11.673559 # passed all 7 test(s)
> > 11.673628 1..7
> 
> Interesting, I get very different results here. Are you using a
> traditional HDD? Since setup.sh does a lot of disk I/O, I could imagine
> a significant slowdown with physical disks. Patches are always welcome!

That’s right, and my hard disk is harldy new, so I’m more likely to
notice slowness. Still, rendercomment should hardly perform any disk I/O
at all, except for Python’s module loading.

Here’s from my /tmp ramdisk:

	$ ~/dev/aurweb/test/t2600-rendercomment.t | ts -s %.s
	0.000010 Starting setup.sh
	0.383917 Compeleted setup.sh
	0.720444 ok 1 - Test comment rendering.
	1.050310 ok 2 - Test Markdown conversion.
	1.385062 ok 3 - Test HTML sanitizing.
	1.721737 ok 4 - Test link conversion.
	2.055162 ok 5 - Test Git commit linkification.
	2.384108 ok 6 - Test Flyspray issue linkification.
	2.710699 ok 7 - Test headings lowering.
	2.712725 # passed all 7 test(s)
	2.712772 1..7

Impressive. I’m pretty sure a large part of these 3 seconds is caused by
process spawning slowness. If the tests were written with pytest instead
of sharness, therefore using a single process, we might be able to reach
sub-second run time.

By the way, all my measures are for hot runs.

I should run all my tests from /tmp, now that they support being run
from anywhere. We could make the test suite use /tmp automatically, but
I don’t think that’s worth it, especially since it’s not desirable for
everyone.

Anyway, to get back to the topic, common initialization could be more
selective. We could update setup.sh so that, say, `./setup.sh database
git` only initializes an empty database and git, but not SSH or specific
database data. That’s kind of equivalent to splitting setup.sh though.

Concerning Python, we won’t be able to call setup.sh and call it a day.
setup.sh includes sharness, which we don’t want. It also sets bash
variables that we won’t be able to see in Python. I’m sure that with
some ingenuity we’ll find a way to concile both where it makes sense.
Frédéric Mangano-Tarumi April 17, 2020, 8:30 p.m. UTC | #12
Filipe Laíns [2020-04-16 22:58:47 +0100]
> On Sun, 2020-04-12 at 18:55 +0200, Frédéric Mangano-Tarumi wrote:
> > +        assert parts.path in ("", "/")
> 
> Why a tuple?

In Python, tuples are essentially immutable lists, and I think it’s a
good practice to use immutable structures when they fit. It makes the
intention clear and occasionally leads to significant optimizations.
Lukas Fleischer May 23, 2020, 4:35 p.m. UTC | #13
On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> Though barely useful in its current state, it shows how to integrate
> pytest with SQLAlchemy and Werkzeug, providing a test framework for the
> potential future Flask port, while actually testing the current PHP
> implementation.
> ---
>  aurweb/test/__init__.py |  0
>  aurweb/test/conftest.py | 51 +++++++++++++++++++++++++++++++++++++++++
>  aurweb/test/test_rpc.py | 21 +++++++++++++++++
>  aurweb/test/wsgihttp.py | 38 ++++++++++++++++++++++++++++++
>  test/README.md          |  3 +++
>  test/rpc.t              |  2 ++
>  6 files changed, 115 insertions(+)
>  create mode 100644 aurweb/test/__init__.py
>  create mode 100644 aurweb/test/conftest.py
>  create mode 100644 aurweb/test/test_rpc.py
>  create mode 100644 aurweb/test/wsgihttp.py
>  create mode 100755 test/rpc.t

This currently seems to break our tests:

    configparser.NoSectionError: No section: 'database'

Did not yet investigate further but that's something that likely needs
to be addressed in some way.
Frédéric Mangano-Tarumi May 23, 2020, 4:58 p.m. UTC | #14
Lukas Fleischer [2020-05-23 12:35:46 -0400]
> On Sun, 12 Apr 2020 at 12:55:46, Frédéric Mangano-Tarumi wrote:
> > ---
> >  aurweb/test/__init__.py |  0
> >  aurweb/test/conftest.py | 51 +++++++++++++++++++++++++++++++++++++++++
> >  aurweb/test/test_rpc.py | 21 +++++++++++++++++
> >  aurweb/test/wsgihttp.py | 38 ++++++++++++++++++++++++++++++
> >  test/README.md          |  3 +++
> >  test/rpc.t              |  2 ++
> 
> This currently seems to break our tests:
> 
>     configparser.NoSectionError: No section: 'database'

This is the version that requires an AUR instance running, and also
AUR_CONFIG to be set to that instance’s config. Note that you need to
set aur_location accordingly too.

The fix consists in making the test suite spawn its own test instance,
as we talked about earlier. Like you suggested, we should also move it
in a separate test directory so that it won’t get triggered by default.

I submitted this patch when we were considering porting the whole
PHP code base to Python. However, if we instead port the code as we make
new features, then the new framework would provide testing tools without
requiring us to prepare and run a dual-stack instance. Functional HTTP
testing won’t be as useful.

I suggest we put this patch on the back burner until we gather the will
manpower to intensively port the code with the intention of keeping its
behavior intact.
Lukas Fleischer May 23, 2020, 8:42 p.m. UTC | #15
On Sat, 23 May 2020 at 12:58:32, Frédéric Mangano-Tarumi wrote:
> I submitted this patch when we were considering porting the whole
> PHP\u202fcode base to Python. However, if we instead port the code as we make
> new features, then the new framework would provide testing tools without
> requiring us to prepare and run a dual-stack instance. Functional HTTP
> testing won\u2019t be as useful.
> 
> I suggest we put this patch on the back burner until we gather the will
> manpower to intensively port the code with the intention of keeping its
> behavior intact.

Thanks for the context. I agree with your analysis and dropped the patch
from pu. We can merge it again in the future if we think it makes more
sense.

Patch

diff --git a/aurweb/test/__init__.py b/aurweb/test/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/aurweb/test/conftest.py b/aurweb/test/conftest.py
new file mode 100644
index 00000000..49cc2f6e
--- /dev/null
+++ b/aurweb/test/conftest.py
@@ -0,0 +1,51 @@ 
+"""
+Fixtures for pytest.
+
+This module is automatically loaded by pytest.
+"""
+
+import pytest
+import sqlalchemy
+import werkzeug.test
+import werkzeug.wrappers
+import werkzeug.wrappers.json
+
+import aurweb.config
+import aurweb.db
+from aurweb.test.wsgihttp import WsgiHttpProxy
+
+
+class Response(werkzeug.wrappers.CommonResponseDescriptorsMixin,
+               werkzeug.wrappers.json.JSONMixin,
+               werkzeug.wrappers.BaseResponse):
+    """
+    Custom response object to be returned by the test client. More mixins could
+    be added if need be.
+
+    See https://werkzeug.palletsprojects.com/en/1.0.x/wrappers/#mixin-classes
+    """
+    pass
+
+
+@pytest.fixture
+def client():
+    """
+    Build a Werkzeug test client for making HTTP requests to AUR. It requires
+    that the AUR test website is already running at `[options] aur_location`,
+    specified in the configuration file.
+
+    When aurweb becomes a pure Flask application, this should return Flask’s
+    test_client(), which is a Werkzeug test client too.
+    https://flask.palletsprojects.com/en/1.1.x/testing/#the-testing-skeleton
+    """
+    base_uri = aurweb.config.get("options", "aur_location")
+    proxy = WsgiHttpProxy(base_uri)
+    return werkzeug.test.Client(proxy, Response)
+
+
+@pytest.fixture(scope="session")
+def db_engine():
+    """
+    Return an SQLAlchemy engine to the configured database.
+    """
+    return sqlalchemy.create_engine(aurweb.db.get_sqlalchemy_url())
diff --git a/aurweb/test/test_rpc.py b/aurweb/test/test_rpc.py
new file mode 100644
index 00000000..7079145c
--- /dev/null
+++ b/aurweb/test/test_rpc.py
@@ -0,0 +1,21 @@ 
+"""
+Test suite for the RPC interface.
+
+See also `doc/rpc.txt` for the RPC interface documentation.
+"""
+
+import pytest
+from sqlalchemy.sql import select
+
+from aurweb.schema import Packages
+
+
+def test_search_by_name(client, db_engine):
+    """Take a package from the database, and find it through the RPC interface."""
+    with db_engine.connect() as conn:
+        pkg = conn.execute(select([Packages]).limit(1)).fetchone()
+        if pkg is None:
+            pytest.skip("needs at least one package in the database")
+    resp = client.get("/rpc/", query_string={"v": "5", "type": "search", "arg": pkg["Name"]})
+    result = resp.json
+    assert result["resultcount"] >= 1
diff --git a/aurweb/test/wsgihttp.py b/aurweb/test/wsgihttp.py
new file mode 100644
index 00000000..5b9d8040
--- /dev/null
+++ b/aurweb/test/wsgihttp.py
@@ -0,0 +1,38 @@ 
+import http.client
+import urllib.parse
+
+
+class WsgiHttpProxy:
+    """
+    WSGI-to-HTTP proxy, that is to say a WSGI application that forwards every
+    WSGI request to an HTTP server, then the HTTP response back to WSGI.
+
+    The base URL the constructor takes is something like
+    `http://localhost:8080`. It must not contain a path, a query string or a
+    fragment, as the proxy wouldn’t now what to do with it.
+
+    Only the HTTP scheme is supported, but HTTPS could probably be easily added.
+    """
+
+    def __init__(self, base_url):
+        parts = urllib.parse.urlsplit(base_url)
+        self.netloc = parts.netloc
+        # Limitations of this dumb proxy
+        assert parts.scheme == "http"
+        assert parts.path in ("", "/")
+        assert parts.query == ""
+        assert parts.fragment == ""
+
+    def __call__(self, environ, start_response):
+        conn = http.client.HTTPConnection(self.netloc)
+        conn.request(
+            method=environ["REQUEST_METHOD"],
+            url=urllib.parse.urlunsplit((
+                "http", self.netloc,
+                urllib.parse.quote(environ["PATH_INFO"]),
+                environ["QUERY_STRING"], "")),
+            body=environ["wsgi.input"].read(int(environ.get("CONTENT_LENGTH", 0))),
+        )
+        resp = conn.getresponse()
+        start_response(f"{resp.status} {resp.reason}", resp.getheaders())
+        return resp
diff --git a/test/README.md b/test/README.md
index de7eff18..cc8baf33 100644
--- a/test/README.md
+++ b/test/README.md
@@ -20,8 +20,11 @@  For all the test to run, the following Arch packages should be installed:
 - python-bleach
 - python-markdown
 - python-pygit2
+- python-pytest
+- python-pytest-tap
 - python-sqlalchemy
 - python-srcinfo
+- python-werkzeug
 
 Writing tests
 -------------
diff --git a/test/rpc.t b/test/rpc.t
new file mode 100755
index 00000000..f950f7df
--- /dev/null
+++ b/test/rpc.t
@@ -0,0 +1,2 @@ 
+#!/bin/sh
+pytest --tap-stream "$(dirname "$0")/../aurweb/test/test_rpc.py"