Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add unit tests for wrapper handlers in tools/serve #14004

Merged
merged 3 commits into from
Nov 12, 2018
Merged

Add unit tests for wrapper handlers in tools/serve #14004

merged 3 commits into from
Nov 12, 2018

Conversation

qiuzhong
Copy link
Contributor

@qiuzhong qiuzhong commented Nov 9, 2018

Add unit tests for all kinds of wrapper handlers in tools/serve/serve.py.
See #12452

jgraham
jgraham requested changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing, thanks!

Only a couple of minor style fixes to request.

tools/wptserve/tests/functional/test_handlers.py Outdated
@@ -8,8 +8,11 @@
from six.moves.urllib.error import HTTPError

wptserve = pytest.importorskip("wptserve")
from .base import TestUsingServer, TestUsingH2Server, doc_root
from .base import TestUsingServer, TestUsingH2Server, \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use continuation characters, please.

tools/wptserve/tests/functional/test_handlers.py Outdated
@@ -388,5 +391,64 @@ def test_requesting_multiple_resources(self):
assert resp.read() == ''


class TestWorkersHandler(TestWrapperHandlerUsingServer):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blank line after the class definition.

@sideshowbarker
Copy link
Contributor

Travis unittest failures:

_________ ERROR collecting wptserve/tests/functional/test_handlers.py __________
ImportError while importing test module '/home/travis/build/web-platform-tests/wpt/tools/wptserve/tests/functional/test_handlers.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
.tox/py27/lib/python2.7/site-packages/six.py:709: in exec_
    exec("""exec _code_ in _globs_, _locs_""")
wptserve/tests/functional/test_handlers.py:14: in <module>
    from serve.serve import WorkersHandler, WindowHandler, AnyHtmlHandler,\
E   ImportError: No module named serve

and

ERROR: InvocationError for command '/home/travis/build/web-platform-tests/wpt/tools/.tox/py36/bin/pytest --cov' (exited with code 2)
py36-flake8 create: /home/travis/build/web-platform-tests/wpt/tools/.tox/py36-flake8
py36-flake8 installdeps: -rrequirements_flake8.txt
py36-flake8 installed: flake8==3.6.0,flake8-polyfill==1.0.2,mccabe==0.6.1,pep8-naming==0.7.0,pycodestyle==2.4.0,pyflakes==2.0.0
py36-flake8 run-test-pre: PYTHONHASHSEED='3166492845'
py36-flake8 runtests: commands[0] | flake8 --append-config=py36-flake8.ini
___________________________________ summary ____________________________________
ERROR:   py36: commands failed

@qiuzhong
Add unit tests for wrapper handlers in tools/serve
a3078b3 
Add unit tests for all kinds of wrapper handlers in tools/serve/serve.py.
See web-platform-tests#12452
@qiuzhong
Update the unit tests
f4bd28a 
* Fix the import error
* Improve the code style
@sideshowbarker
Copy link
Contributor

sideshowbarker commented Nov 12, 2018 •
edited
Loading

See https://travis-ci.org/web-platform-tests/wpt/jobs/453768016; unittests still failing under Python3

@qiuzhong
Read and write files in binary mode
c4401e4 
Read and write files in binary mode so the unit tests can pass in
Python3 environment.
@qiuzhong
Copy link
Contributor Author

@jgraham , thanks for your comments. I improved the code style.

@sideshowbarker , the import error and other errors have been fixed.

@sideshowbarker
Copy link
Contributor

Yeah, c4401e4 is all green now, so when the review is approved, this should be ready to merge to master

@jgraham jgraham merged commit d0c94e6 into web-platform-tests:master Nov 12, 2018
@jugglinmike jugglinmike mentioned this pull request Nov 14, 2018
Closed
@qiuzhong qiuzhong mentioned this pull request Dec 3, 2018
Closed
@jugglinmike jugglinmike mentioned this pull request Dec 15, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgraham jgraham jgraham approved these changes

@gsnedders gsnedders gsnedders approved these changes

@jugglinmike jugglinmike Awaiting requested review from jugglinmike

Assignees
No one assigned
Labels
infra wptserve
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@qiuzhong @sideshowbarker @gsnedders @jgraham @wpt-pr-bot