Feature/ted 84 #15
Feature/ted 84 #15
Conversation
Codecov Report
@@ Coverage Diff @@
## main #15 +/- ##
==========================================
+ Coverage 95.21% 95.80% +0.58%
==========================================
Files 19 20 +1
Lines 355 381 +26
==========================================
+ Hits 338 365 +27
+ Misses 17 16 -1
Continue to review full report at Codecov.
|
requirements.txt
Outdated
| python-dotenv~=0.19.2 | ||
| pymongo~=4.0.1 | ||
| mongomock==4.0.0 |
There was a problem hiding this comment.
mongomock should go into the requirements -dev.txt
|
|
||
| def __init__(self,mongodb_client: MongoClient, database_name: str = None): | ||
| mongodb_client = mongodb_client | ||
| notice_db = mongodb_client[database_name if database_name else self._database_name] |
There was a problem hiding this comment.
We need to check how safe it is to assume that we can refer to a DB even in cases it does not exist. This shall be tested.
mongodb_client['db_name'] might not be a safe operation.
| :return: | ||
| """ | ||
| notice_dict = notice.dict() | ||
| notice_dict["_id"] = notice_dict["ted_id"] |
There was a problem hiding this comment.
I propose to move this logic into the model directly.
We could "export" (.to_dict) an `_id' key that is equal to the ted_id.
| :param reference: | ||
| :return: Notice | ||
| """ | ||
| result_dict = self.collection.find_one({"ted_id": reference}) |
There was a problem hiding this comment.
we would have to standardise if we search based on _id or ted_id
There was a problem hiding this comment.
_id == ted_id
| This method allows all records to be retrieved from the repository. | ||
| :return: list of notices | ||
| """ | ||
| return [Notice(**result_dict) for result_dict in self.collection.find()] |
There was a problem hiding this comment.
Notice(**result_dict) is partially safe at the moment.
What happens to the _id for example? If we do not care, then it should be fairly okay. But still needs to be tested in e2e tests.
Also, what happens in the case of very very large databases? Shall we provide it as a cursor or a generator instead of a memory-hungry list? I would recommend the generator option. But need to check what is the general practice.
tests/conftest.py
Outdated
| @@ -3,7 +3,7 @@ | |||
| from ted_sws.notice_fetcher.adapters.ted_api import TedAPIAdapter | |||
| from tests.fakes.fake_repository import FakeNoticeRepository | |||
| from tests.fakes.fake_ted_api import FakeRequestAPI | |||
|
|
|||
| from pymongo import MongoClient | |||
There was a problem hiding this comment.
I would recommend removing e2e requirements from the tests/conftest.py and moving it closer to the tests where it is used i.e. e2e/conftest.py tests for example.
|
|
||
| @pytest.fixture | ||
| def mongodb_client(): | ||
| uri = config.MONGO_DB_AUTH_URL |
There was a problem hiding this comment.
Why ...AUTH_URL ?
shouldn't we receive from the env the
- (a) database URL in one variable,
- (b) the auth parameters in another one or two, depending on the method
|
|
||
|
|
||
| @pytest.fixture | ||
| @mongomock.patch(servers=(('server.example.com', 27017),)) |
There was a problem hiding this comment.
We are gonna have to decide whether we use Fakes or mocks.
Generally, we do not need both unless I am ignorant or missing something.
There was a problem hiding this comment.
for pymongo we need mock approach
| TEST_DB_NAME = 'test_db' | ||
|
|
||
|
|
||
| def test_notice_repository_create(mongodb_client): |
There was a problem hiding this comment.
This test is identical to the other one in e2e.
| :return: list of notices | ||
| """ | ||
| for result_dict in self.collection.find(): | ||
| yield Notice(**result_dict) |
There was a problem hiding this comment.
Clever!
I only hope that self.collection.find() is also a generator.
|
BTW the tests are failing :) |
No description provided.