feat: Database create with description #93
Conversation
…e method; add test for database description
tests/unit/conftest.py
Outdated
| @pytest.fixture | ||
| def mock_db_name() -> str: | ||
| return "mock_database" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def db_description() -> str: | ||
| return "database description" | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_db_description() -> str: | ||
| return "mock database description" | ||
|
|
There was a problem hiding this comment.
What's the difference between db_name, db_description and their mock counterparts?
There was a problem hiding this comment.
db_name (and db_description) fixtures are used everywhere, which leads to cases, when the parameter passing is not being checked correctly. For example in tests/unit/service/test_database.py:33, previously if we would pass something different to the database = manager.databases.create(name="other_db_name"), the next check assert database.name == db_name would still work correctly, which is a wrong behaviour.
In my opinion, all listed fixtures are unnecessary, however removing them would lead to a lot of refactoring.
There was a problem hiding this comment.
I'm not sure how changing the name in the mock fixes the issue you're outlining. In test_database.py:33 when a new name is passed to manager.databases.create nothing changes since httpx still has the databases url callback mocked to return the result with db_name, which passes the assert below. If that's modified to use mock_db_name the issue still stands e.g. manager.databases.create(name="yet_another_db_name") would pass the assert database.name == "other_db_name".
I think in order to fix this and avoid any mistakes made in testing we can check what's being sent to the mock API in the test itself. e.g. httpx_mock.get_request( <insert_expected_request_here> )
There was a problem hiding this comment.
After decoupling the parameters, the test_databases_create started to fail, showing us that, there is an issue. Having just one db_name, that we use everywhere obfuscates such problems.
By splitting db_name and mock_db_name I didn't fix the issue. The issue was fixed by extending the create_databases_callback.
There was a problem hiding this comment.
This seems like we're testing the fixture and not the sdk itself.
My suggestion would be to remove the mock_database fixture from the callback function and create another database object inside a callback with provided parameters, and then return it's dict(). This way it seems to be simpler and representative.
Depending on different database names seems too implicit and not very obvious
There was a problem hiding this comment.
As suggested by @stepansergeevitch, I would remove both use mock_database as a function, then the mock_db_name mock_db_description would not be needed anymore, and I will remove them as well. @ptiurin are you fine with this approach?
There was a problem hiding this comment.
I figured, that mock_database is used much more than I thought, so this solution is also not optimal. I will try to find something more elegant
| ) -> Response: | ||
| database_properties = json.loads(request.read().decode("utf-8"))["database"] | ||
|
|
||
| mock_database.name = database_properties["name"] |
There was a problem hiding this comment.
I think that we can just return database_properties as json parameter in to_response
There was a problem hiding this comment.
the mock_database fixture set some additional default parameters, e.g. database_id. I would prefer to keep the responsibility for the default parameters to the mock_database function.
…t-db/firebolt-python-sdk into database-create-with-description
|
Kudos, SonarCloud Quality Gate passed! |
No description provided.