-
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
Changes from all commits
857f44e
268a4ec
11f7e27
e609955
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
{ | ||
"version": "0.2.0", | ||
"configurations": [ | ||
{ | ||
"name": "PyTest All", | ||
"type": "python", | ||
"request": "launch", | ||
"module": "pytest", | ||
"justMyCode": true | ||
}, | ||
{ | ||
"args": [ | ||
"--nf", | ||
"--lf" | ||
], | ||
"name": "PyTest New and Failing", | ||
"type": "python", | ||
"request": "launch", | ||
"module": "pytest", | ||
"justMyCode": true | ||
}, | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,10 +123,10 @@ def add(self, item: Prediction) -> Tuple[bool, Any]: | |
correlation_id=item.get("correlationId") or item.get("correlation_id"), | ||
data=item.get("data"), | ||
model_id=item.get("modelId") or item.get("model_id"), | ||
receiver_name=receiver.get("name"), | ||
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ahh yeah, my misstake |
||
receiver_role=receiver.get("role") if receiver else None, | ||
sender_name=sender.get("name") if sender else None, | ||
sender_role=sender.get("role") if sender else None, | ||
prediction_id=item.get("predictionId") or item.get("prediction_id"), | ||
timestamp=item.get("timestamp"), | ||
) | ||
|
@@ -187,8 +187,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: | ||
stmt = stmt.offset(skip or 0).limit(limit) | ||
elif skip: | ||
stmt = stmt.offset(skip) | ||
|
||
items = session.execute(stmt) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
|
||
import sys | ||
import pytest | ||
|
||
from fedn.tests.stores.helpers.database_helper import mongo_connection, sql_connection, postgres_connection | ||
|
||
|
||
# These lines ensure that pytests trigger breakpoints when assertions fail during debugging | ||
def is_debugging(): | ||
return 'debugpy' in sys.modules | ||
|
||
# enable_stop_on_exceptions if the debugger is running during a test | ||
if is_debugging(): | ||
@pytest.hookimpl(tryfirst=True) | ||
def pytest_exception_interact(call): | ||
raise call.excinfo.value | ||
|
||
@pytest.hookimpl(tryfirst=True) | ||
def pytest_internalerror(excinfo): | ||
raise excinfo.value |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
import pytest | ||
from unittest.mock import patch | ||
|
||
from fedn.network.storage.dbconnection import DatabaseConnection | ||
from fedn.tests.stores.helpers.mongo_docker import start_mongodb_container, stop_mongodb_container | ||
from fedn.tests.stores.helpers.postgres_docker import start_postgres_container, stop_postgres_container | ||
|
||
|
||
def network_id(): | ||
return "test_network" | ||
|
||
@pytest.fixture(scope="package") | ||
def mongo_connection(): | ||
already_running, _, port = start_mongodb_container() | ||
|
||
def mongo_config(): | ||
return { | ||
"type": "MongoDB", | ||
"mongo_config": { | ||
"host": "localhost", | ||
"port": port, | ||
"username": "fedn_admin", | ||
"password": "password" | ||
} | ||
} | ||
|
||
with patch('fedn.network.storage.dbconnection.get_statestore_config', return_value=mongo_config()), \ | ||
patch('fedn.network.storage.dbconnection.get_network_config', return_value=network_id()): | ||
yield DatabaseConnection(force_create_new=True) | ||
if not already_running: | ||
stop_mongodb_container() | ||
|
||
@pytest.fixture(scope="package") | ||
def sql_connection(): | ||
def sql_config(): | ||
return { | ||
"type": "SQLite", | ||
"sqlite_config": { | ||
"dbname": ":memory:", | ||
} | ||
} | ||
|
||
with patch('fedn.network.storage.dbconnection.get_statestore_config', return_value=sql_config()), \ | ||
patch('fedn.network.storage.dbconnection.get_network_config', return_value=network_id()): | ||
return DatabaseConnection(force_create_new=True) | ||
|
||
@pytest.fixture(scope="package") | ||
def postgres_connection(): | ||
already_running, _, port = start_postgres_container() | ||
|
||
|
||
|
||
def postgres_config(): | ||
return { | ||
"type": "PostgreSQL", | ||
"postgres_config": { | ||
"username": "fedn_admin", | ||
"password": "password", | ||
"database": "fedn_db", | ||
"host": "localhost", | ||
"port": port | ||
} | ||
} | ||
|
||
with patch('fedn.network.storage.dbconnection.get_statestore_config', return_value=postgres_config()), \ | ||
patch('fedn.network.storage.dbconnection.get_network_config', return_value=network_id()): | ||
yield DatabaseConnection(force_create_new=True) | ||
if not already_running: | ||
stop_postgres_container() |
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.