Add pytest for integration testing tornado

RESOLVED FIXED

Status

Conduit
General
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: mars, Assigned: mars)

Tracking

Details

(URL)

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

10 months ago
Story card: https://trello.com/c/LPWm3kTj
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

10 months ago
mozreview-review
Comment on attachment 8834621 [details]
autoland: include test dependencies in autoland service container (bug 1337501)

https://reviewboard.mozilla.org/r/110480/#review111994

::: autoland/public-web-api/Dockerfile-dev:4
(Diff revision 1)
> -RUN pip install -r /requirements.txt
> +ADD test-requirements.txt /test-requirements.txt
> +RUN pip install -r /test-requirements.txt

The test-requirements.txt is in addition to the requirements.txt, not a replacement. You still need to install the requirements.txt file.
Attachment #8834621 - Flags: review?(smacleod) → review-

Comment 5

10 months ago
mozreview-review
Comment on attachment 8834623 [details]
autoland: add integration tests for Dockerflow api calls (bug 1337501)

https://reviewboard.mozilla.org/r/110484/#review112002

::: autoland/public-web-api/test-requirements.txt:1
(Diff revision 1)
> -flake8
> +-r requirements.txt

Ah, so this is why your code works. Your first commit is invalid without this. I still think I'd prefer to keep the invocations separate.

::: autoland/public-web-api/test-requirements.txt:4
(Diff revision 1)
> -flake8
> -pytest
> -yapf
> +-r requirements.txt
> +
> +py==1.4.32 \
> +    --hash=sha256:2d4bba2e25fff58140e6bdce1e485e89bb59776adbe01d490baa6b1f37a3dd6b

Since these requirements weren't going to be used in production, I thought making it easy to test with the latest would be nice - I guess your way keeps the environment consistent though... thoughts?

::: autoland/public-web-api/test-requirements.txt:12
(Diff revision 1)
> +tornado==4.4.2 \
> +    --hash=sha256:2898f992f898cd41eeb8d53b6df75495f2f423b6672890aadaf196ea1448edcc

I'd rather not include the actual app requirements themselves in this file - you're duplicating this requirement (check others please)

::: autoland/public-web-api/tests/test_ops_api.py:21
(Diff revision 1)
> +def app():
> +    return make_app(False)
> +
> +
> +@pytest.mark.gen_test
> +def test_loadbalancer_heartbeat_returns_200(http_client, base_url):

uhhhh.... where are these fixtures? forget to add a file?

::: autoland/public-web-api/tests/test_ops_api.py:21
(Diff revision 1)
> +def app():
> +    return make_app(False)
> +
> +
> +@pytest.mark.gen_test
> +def test_loadbalancer_heartbeat_returns_200(http_client, base_url):

Can we use the `async def` and `await` syntax here? I'd really like us to be consistent if possible.

It looks like https://pypi.python.org/pypi/pytest-asyncio can? Or maybe pytest has something built in now?

::: autoland/public-web-api/tests/test_ops_api.py:29
(Diff revision 1)
> +    assert response.code == 200
> +
> +
> +@pytest.mark.gen_test
> +def test_heartbeat_returns_200(http_client, base_url):
> +    heartbeat_url = base_url + '/__heartbeat__'

This doesn't actually exist yet. How about you just keep it to the single lbheartbeat test?
Attachment #8834623 - Flags: review?(smacleod) → review-

Comment 6

10 months ago
mozreview-review
Comment on attachment 8834622 [details]
autoland: hook for test suite to construct a tornado app object (bug 1337501)

https://reviewboard.mozilla.org/r/110482/#review112004
Attachment #8834622 - Flags: review?(smacleod) → review+
(Assignee)

Comment 7

10 months ago
mozreview-review-reply
Comment on attachment 8834621 [details]
autoland: include test dependencies in autoland service container (bug 1337501)

https://reviewboard.mozilla.org/r/110480/#review111994

> The test-requirements.txt is in addition to the requirements.txt, not a replacement. You still need to install the requirements.txt file.

I included the requirements.txt file as a dependency inside test-requirements.txt using '-r requirements.txt'.  Does that work?
(Assignee)

Comment 8

10 months ago
mozreview-review-reply
Comment on attachment 8834623 [details]
autoland: add integration tests for Dockerflow api calls (bug 1337501)

https://reviewboard.mozilla.org/r/110484/#review112002

> Ah, so this is why your code works. Your first commit is invalid without this. I still think I'd prefer to keep the invocations separate.

Oops, looks like this slipped into the later commit (it should have come with the updates to Dockerfile-dev).  I'll update the earlier commit.

> Since these requirements weren't going to be used in production, I thought making it easy to test with the latest would be nice - I guess your way keeps the environment consistent though... thoughts?

I wanted to keep things consistent across of the different instances of dev and test environments we will have.  It's the easiest way to get reproducible image builds and reproducible test runs.

> uhhhh.... where are these fixtures? forget to add a file?

The fixtures come from pytest-tornado.  You can run 'pytest --fixtures' to get the full list.

> Can we use the `async def` and `await` syntax here? I'd really like us to be consistent if possible.
> 
> It looks like https://pypi.python.org/pypi/pytest-asyncio can? Or maybe pytest has something built in now?

Do you mean using the async primatives instead of `yield`?

`@pytest.mark.gen_test` handles the async io control.  From what I saw of the examples online, people don't use the async primatives in tornado tests.  I'm not saying we can't, just that I didn't see any examples to tell me if `async def` and `await` would work or not.

Comment 9

10 months ago
mozreview-review-reply
Comment on attachment 8834623 [details]
autoland: add integration tests for Dockerflow api calls (bug 1337501)

https://reviewboard.mozilla.org/r/110484/#review112002

> I wanted to keep things consistent across of the different instances of dev and test environments we will have.  It's the easiest way to get reproducible image builds and reproducible test runs.

ya, sounds good to me.

> Do you mean using the async primatives instead of `yield`?
> 
> `@pytest.mark.gen_test` handles the async io control.  From what I saw of the examples online, people don't use the async primatives in tornado tests.  I'm not saying we can't, just that I didn't see any examples to tell me if `async def` and `await` would work or not.

Yes, please use the primitives. The library I linked lets you use it at least?

Comment 10

10 months ago
mozreview-review
Comment on attachment 8834623 [details]
autoland: add integration tests for Dockerflow api calls (bug 1337501)

https://reviewboard.mozilla.org/r/110484/#review112172

::: autoland/public-web-api/test-requirements.txt:28
(Diff revision 1)
> +
> +mccabe==0.5.3 \
> +    --hash=sha256:f9b58bf366c1506dcd6117b33e5c4874746f0de859c9c7cab8b516cb6be1d22e
> +
> +pytest==3.0.6 \
> +    --hash=sha256:da0ab50c7eec0683bc24f1c1137db1f4111752054ecdad63125e7ec71316b813

Please include all the valid hashes - https://pypi.python.org/pypi/hashin can make this easier.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

10 months ago
mozreview-review-reply
Comment on attachment 8834623 [details]
autoland: add integration tests for Dockerflow api calls (bug 1337501)

https://reviewboard.mozilla.org/r/110484/#review112002

> Yes, please use the primitives. The library I linked lets you use it at least?

Discussed on IRC, we'll do a follow-up patch with the changes.

Comment 15

10 months ago
mozreview-review
Comment on attachment 8834621 [details]
autoland: include test dependencies in autoland service container (bug 1337501)

https://reviewboard.mozilla.org/r/110480/#review112180
Attachment #8834621 - Flags: review?(smacleod) → review+

Comment 16

10 months ago
mozreview-review
Comment on attachment 8834623 [details]
autoland: add integration tests for Dockerflow api calls (bug 1337501)

https://reviewboard.mozilla.org/r/110484/#review112182

::: autoland/public-web-api/tests/test_ops_api.py:1
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public

bah, missed this the first time - can we just call this `test_dockerflow.py`? I think that's more descriptipve.
Attachment #8834623 - Flags: review?(smacleod) → review+
Comment hidden (mozreview-request)

Comment 18

10 months ago
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/automation/conduit/rev/53cd57c5383f
autoland: include test dependencies in autoland service container r=smacleod
https://hg.mozilla.org/automation/conduit/rev/fea3b864df4b
autoland: hook for test suite to construct a tornado app object r=smacleod
https://hg.mozilla.org/automation/conduit/rev/b73f3c6e8faa
autoland: add integration tests for Dockerflow api calls r=smacleod
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED

Updated

9 months ago
Assignee: nobody → mars
You need to log in before you can comment on or make changes to this bug.