-
Notifications
You must be signed in to change notification settings - Fork 0
Persist and test API namespace for projects #40
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
Conversation
| api.init_app(app) | ||
|
|
||
| if app.config.get('DEBUG'): | ||
| app.logger.setLevel(logging.INFO) |
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.
should not be app.logger.setLevel(logging.DEBUG)?
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.
IMHO logging.INFO is detailed enough.
| SECRET_KEY = generate_dev_secret_key() | ||
| DATABASE_URI = os.environ.get('DATABASE_URI') | ||
| PROPAGATE_EXCEPTIONS = True | ||
| RESTPLUS_VALIDATE = True |
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.
Good.
|
|
||
|
|
||
| def test_list_all_projects(client: FlaskClient, mocker: MockFixture): | ||
| from time_tracker_api.projects.projects_namespace import project_dao |
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.
Notice, you are calling from time_tracker_api.projects.projects_namespace import project_dao in all the tests.
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.
Yes, if I do it before the project_dao would be broken because it has to be constructed after the app instance is ready. This is a necessary evil.
| repository_create_mock.assert_called_once_with(valid_project_data) | ||
|
|
||
|
|
||
| def test_create_project_should_reject_bad_request(client: FlaskClient, mocker: MockFixture): |
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 think we can use the name test_create_project_should_fail_with_invalid_request for consistency with the previous request.
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 rather to leave bad request because that is the verbose name of the HTTP status code 400.
| repository_create_mock.assert_not_called() | ||
|
|
||
|
|
||
| def test_list_all_projects(client: FlaskClient, mocker: MockFixture): |
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.
What about test_get_all_projects or test_get_projects?
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.
The description in the comments says list. As long as it makes sense and it is not ambiguous we are Ok.
| repository_find_mock.assert_called_once_with(str(invalid_id)) | ||
|
|
||
|
|
||
| def update_project_should_succeed_with_valid_data(client: FlaskClient, mocker: MockFixture): |
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.
You are missing the test_ prefix.
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.
Nice catch
| repository_update_mock.assert_called_once_with(valid_id, valid_project_data) | ||
|
|
||
|
|
||
| def test_update_project_should_reject_bad_request(client: FlaskClient, mocker: MockFixture): |
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 think we can use the name test_update_project_should_fail_with_invalid_request as before.
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.
Bad request is better
e7ee5f6 to
e8e225a
Compare
e8e225a to
4ce4612
Compare
This PR:
This will set the foundation for future tests of the persistence of API namespaces, covering 100% of it.