-
Notifications
You must be signed in to change notification settings - Fork 35
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
Feature/SK-1289 | Added test for store list functionality #808
Conversation
…ests failed. Also added helpers to setup the tests using pip install docker
receiver_role=receiver.get("role"), | ||
sender_name=sender.get("name"), | ||
sender_role=sender.get("role"), | ||
receiver_name=receiver.get("name") if receiver else None, |
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 get method returns None if not present no? At least I think you can set a default: get("name", None)... But I might be misstaken...
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.
If reciever is None you can't call .get() on it. That's the problem
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.
Ahh yeah, my misstake
@@ -238,8 +238,10 @@ def list(self, limit: int, skip: int, sort_key: str, sort_order=pymongo.DESCENDI | |||
|
|||
stmt = stmt.order_by(sort_obj) | |||
|
|||
if limit != 0: | |||
if limit: |
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.
We could perhaps check so that these values are positive... (This and similar)
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.
Yeah, it might be a good idea. For now I was happy with just supporting the current usage. It seemed that both None and 0 was used interchangably there so I figure that the code should support that
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.
This too is due to how the stores have been used (only via the routes). Previously we have programatically ensured limit will never be null... This is an improvement as is, do with the comment what you will.
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.
Sweet but can this not be done with a class based approach? Similar to how we have the store set up I imagine... The stores should work similar across different entities so there should be some common ground. It's a thought but I think I could help when adding when adding tests for updating, deleting etc...
This pull request includes various changes to improve the testing setup, database connection handling. The most important changes include adding new VS Code configurations for testing, updating database connection methods.