Closed Bug 1337501 Opened 7 years ago Closed 7 years ago

Add pytest for integration testing tornado

Categories

(Conduit :: General, defect)

Product:
Conduit
Component:
General
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mars, Assigned: mars)

References

(
URL
)

Details

Attachments

(3 files)

autoland: include test dependencies in autoland service container (bug 1337501)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: hook for test suite to construct a tornado app object (bug 1337501)
59 bytes, text/x-review-board-request
smacleod
: review+
Details
autoland: add integration tests for Dockerflow api calls (bug 1337501)
59 bytes, text/x-review-board-request
smacleod
: review+
Details 
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 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 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+
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?
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 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 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 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 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 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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Assignee: nobody → mars
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: