Closed
Bug 1337501
Opened 7 years ago
Closed 7 years ago
Add pytest for integration testing tornado
Categories
(Conduit :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mars, Assigned: mars)
References
()
Details
Attachments
(3 files)
Story card: https://trello.com/c/LPWm3kTj
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Assignee: nobody → mars
You need to log in
before you can comment on or make changes to this bug.
Description
•