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.

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

Raise TypeError if port is not an integer #1088

Merged
merged 3 commits into from Mar 27, 2017

Conversation

Projects
None yet
4 participants
@aaossa
Copy link
Contributor

commented Mar 25, 2017

As explained in pallets/flask#2220 port should be an integer and in other cases a TypeError exception should be raised. This PR includes a type check to raise said exception and a test to check if the exception is raised (when use_reloader is True and when its False).

Closes pallets/flask#2220



def test_port_must_be_integer(dev_server):
with pytest.raises(TypeError) as port_type_exception:

This comment has been minimized.

Copy link
@untitaker

untitaker Mar 25, 2017

Member

Unfortunately this sort of test doesn't work because the server will be started in an entirely different process. The TypeError does happen, but you won't catch it here. Instead I think you should call run_simple directly.

This comment has been minimized.

Copy link
@aaossa

aaossa Mar 25, 2017

Author Contributor

Thanks! I was wondering why there was a RuntimeError 👌


def test_port_must_be_integer(dev_server):
with pytest.raises(TypeError) as port_type_exception_with_reloader:
def app(environ, start_response):

This comment has been minimized.

Copy link
@JordanP

JordanP Mar 25, 2017

You could define this app function in the outer scope (i.e not in the context manager) to reuse in your 2 tests.

This comment has been minimized.

Copy link
@aaossa

aaossa Mar 25, 2017

Author Contributor

Good idea 🙌

@aaossa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2017

Thanks for your review @JordanP and @untitaker . The new test is ready and works 👌

start_response('200 OK', [('Content-Type', 'text/html')])
return [b'hello']

with pytest.raises(TypeError) as port_type_exception_with_reloader:

This comment has been minimized.

Copy link
@untitaker

untitaker Mar 27, 2017

Member

Just rename port_type_exc... to excinfo or something like that.

@untitaker untitaker changed the base branch from 0.12-maintenance to master Mar 27, 2017

@untitaker

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

Please rebase and add a changelog to version 0.13.

0.12-maintenance is really just for bugfixes, but this actually changes behavior.

aaossa added some commits Mar 25, 2017

Check port parameter type at run_simple
Ports should be integers, so a type check is explicitly added to raise
an exception when this is ignored. This inappropiate behaviour was
detected when using `debug=True` in a Flask application, because a
string is accepted and works when it should fail.

Closes pallets/flask#2220

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Add testcase for pallets/flask#2220
This testcase tries to use a string as port parameter, which should not
be allowed by `run_simple`. The test uses `run_simple` directly
because the tests configuration raises the error in a different process
without giving a chance to catch the `TypeError` exception.

See pallets/flask#2220

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Add PR #1088 to Changelog
Signed-off-by: Antonio Ossa <aaossa@uc.cl>
@aaossa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2017

I think I've done it 👌 Do you have more issues or PRs to read/work on? 😁

@untitaker untitaker merged commit d2b373a into pallets:master Mar 27, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@untitaker

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

Thanks!

@untitaker

This comment has been minimized.

Copy link
Member

commented Mar 27, 2017

Regarding your question, I think everything labelled with "beginner ready" would work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.