-
Notifications
You must be signed in to change notification settings - Fork 45
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
Additional tests for logout (fixes #206). #207
Additional tests for logout (fixes #206). #207
Conversation
backend/tests/test_logout.py
Outdated
@app.route("/", methods=['GET']) | ||
@jwt_required | ||
def protected(): | ||
return jsonify({"msg": "That it proteced route"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go with "message" (as we would in other situations) I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- "that is", not "that it"
backend/tests/test_logout.py
Outdated
assert len(tokens) == 1 | ||
assert tokens[0].revoked | ||
assert rv.status_code == HTTPStatus.UNAUTHORIZED | ||
assert response["msg"] == "token has been revoked" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is behavior implemented in endpoint (not related to the test). Biside naming convencion for messages, we should not checking if token was revoked or not manually in every endpoint, as it is now (flask_jwt_extended has method to do it autmomatically). I added #208 targeting that problem.
backend/tests/test_logout.py
Outdated
}) | ||
response = rv.get_json() | ||
assert rv.status_code == HTTPStatus.UNAUTHORIZED | ||
assert response["msg"] == "token has been revoked" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
backend/tests/test_logout.py
Outdated
def test_get_protected_route_after_logout( | ||
app, client, protected_route, access_token | ||
): | ||
"""Test access protected route after loggin out.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logging
You have still tests failing here @stanislawK |
@magul I think they should. Bug is in our app, not in tests. That why I added #208 and blocked this pr until issue will be resolved. |
backend/tests/test_logout.py
Outdated
rv = client.delete( | ||
"/auth/logout/", headers={"Authorization": "Bearer {}".format(access_token)} | ||
) | ||
assert rv.status_code == HTTPStatus.OK | ||
rv = client.delete( | ||
"/auth/logout/", headers={"Authorization": "Bearer {}".format(access_token)} | ||
) | ||
tokens = JWTToken.query.all() | ||
response = rv.get_json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: Could you reaname variables on more meaningful? E.g. rv
on response
and response
( in response = rv.get_json()
) on payload
backend/tests/test_logout.py
Outdated
rv = client.delete( | ||
"/auth/logout/", headers={"Authorization": "Bearer {}".format(access_token)} | ||
) | ||
assert rv.status_code == HTTPStatus.OK | ||
rv = client.get("/", headers={"Authorization": "Bearer {}".format(access_token)}) | ||
response = rv.get_json() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
backend/tests/test_logout.py
Outdated
@app.route("/", methods=["GET"]) | ||
@jwt_required | ||
def protected(): | ||
return jsonify({"message": "That is proteced route"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
protected
@stanislawK please resolve conflicts & address comments. |
…tanislawK/codeforpoznan.pl_v3 into 206_additional_test_for_logout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit.
To test:
make start
make test