Skip to content

ref: Do not use test classes for tornado tests#292

Merged
untitaker merged 1 commit into
masterfrom
ref/tornado-remove-test-classes
Mar 18, 2019
Merged

ref: Do not use test classes for tornado tests#292
untitaker merged 1 commit into
masterfrom
ref/tornado-remove-test-classes

Conversation

@untitaker
Copy link
Copy Markdown
Member

@nickbaum you seem to know what you're doing. Can you recommend something to test Tornado apps with? This solution I have right now is working fine but it seems hard to maintain

@untitaker untitaker merged commit d719ce0 into master Mar 18, 2019
@untitaker untitaker deleted the ref/tornado-remove-test-classes branch March 18, 2019 14:04
@nickbaum
Copy link
Copy Markdown

Haha, I know barely enough to be dangerous, but I don't think the AsyncHTTPTestCase requires any async code, it just supports it. The following code seems to work to trigger an exception in the test case, perhaps you see how you'd integrate the Sentry testing into that?

import json
import unittest
import tornado.web
import tornado.testing


class TestHandler(tornado.web.RequestHandler):
    def get(self):
        raise Exception("GET")

    def post(self):
        raise Exception("POST")


def make_app():
    return tornado.web.Application([
        (r"/test", TestHandler),
    ])


class TestCase(tornado.testing.AsyncHTTPTestCase):
    def get_app(self):
        return make_app()

    def test_homepage(self):
        response = self.fetch('/test')
        self.assertEqual(response.code, 500)
        response = self.fetch('/test', method='POST', body=json.dumps({"arg1": "value1"}))
        self.assertEqual(response.code, 500)


if __name__ == '__main__':
    unittest.main()

@bdarnell (Tornado's maintainer) might have some pointers. Ben, Markus is looking for the best way to maintain Sentry's integration with Tornado, if you have any advice.

@untitaker
Copy link
Copy Markdown
Member Author

I'm interested in using pytest fixtures for the tests as opposed to unittest classes, because injecting my existing pytest fixtures into AsyncHTTPTestCase-based tests is painful.

Anyway, what i am doing now works well enough for me, just wondering if there's a better way.

but I don't think the AsyncHTTPTestCase requires any async code

I was talking about pytest-tornado and pytest-asyncio when I said that. I have nothing against async code even, but I just didn't get anything other than what I have right now to run.

@bdarnell
Copy link
Copy Markdown

In general I recommend either fully embracing async (in this case using pytest-tornado or pytest-asyncio. It's not clear what obstacles you ran into there), or keeping it at arms length by running the tornado server on a separate thread (as, I assume, you are doing with the synchronous frameworks). What you have here looks like an awkward middle ground.

I think it's also possible to make a more targeted test fixture for your use case that doesn't rely on AsyncHTTPTestCase. Something like this (I don't have much experience with pytest and I'm not sure how its fixtures work, but it's also not clear that they're necessary here):

def tornado_fetch(app, path):
    async def inner():
        # might need a try/finally for `sock.close()`
        sock, port = tornado.testing.bind_unused_port()
        server = tornado.httpserver.HTTPServer(app)
        server.add_socket(sock)
        client = tornado.httpclient.AsyncHTTPClient()
        return await client.fetch(urllib.parse.urljoin('http://127.0.0.1:%d' % port, path))
    with contextlib.closing(tornado.ioloop.IOLoop()) as loop:
        return loop.run_sync(inner)

tornado_fetch(app, path) should be equivalent to your tornado_testcase(app).fetch(path).

@nickbaum
Copy link
Copy Markdown

Thanks for the reply, @bdarnell!

Markus, I'm not at all familiar with these test frameworks, so I'm afraid I won't be much further help on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants