#123: Added creating a BucketFS connection #124
Conversation
| pointing to the root of the BucketFS bucket. Otherwise, even if the BucketFS access | ||
| is configured, the object is None. | ||
| The `get_bucketfs_location` is not tested here but mocked instead. It is tested in |
There was a problem hiding this comment.
Do you actually need an integration test then. Or, is this integration test a preparation for testing the tools?
There was a problem hiding this comment.
Are you asking if we need the test that has been written (test_mcp_server_with_bucketfs), or if we need an integration test that covers get_bucketfs_location?
If it's the former, yes, I think we needed this test because the subsequent tests of the tools will not necessarily use the mcp_server. The main set of tools' tests - in test_mcp_server.py - uses a shortcut.
If it's the latter, hmm, yes, I feel slightly uneasy with this mock. In theory, it should be all good. The unit test covers this function completely, but still... It's just a bit awkward to extract the bucketfs parameters from backend_aware_bucketfs_params, given by the pytest plugin.
There was a problem hiding this comment.
ok, after another look, I now know what bothers me. For get_bucketfs_location we only have unit tests that check that we parse the environment variable into parameters as expected. However, we don't test if the real infer_path likes these parameters. And this test isn't testing this easier, because it mocks get_bucketfs_location with an already created bucketfs.
There was a problem hiding this comment.
Yeah, that was my concern too.
| @patch("exasol.bucketfs.path.infer_path") | ||
| def test_get_bucketfs_location(mock_infer_path, env, expected_kwargs) -> None: | ||
| get_bucketfs_location(env) | ||
| mock_infer_path.assert_called_with(**expected_kwargs) |
There was a problem hiding this comment.
In theory, we could test the environment parsing also without mocks if we extract it from get_bucketfs_location. Only a thought about how to structure testable code. We don't need to change it. Background, in general the most stable unit tests are input out tests without mocking.
There was a problem hiding this comment.
It makes the code more difficult to read. The mock is fine in my opinion.
test/unit/test_connection_factory.py
Outdated
|
|
||
| def test_get_bucketfs_location_failure(caplog) -> None: | ||
| caplog.clear() | ||
| bucketfs_location = get_bucketfs_location( |
There was a problem hiding this comment.
I might have actually raised an exception and do the logging in the caller. However, we probably can leave it as well.
There was a problem hiding this comment.
I would formulate it as a question of what we should do if the bucketfs is not configured properly, but the bucketfs tools are enabled. Should we blow up the whole thing or log a warning and continue? Since this is not a multi-user configuration, maybe the former is more appropriate. What do you think?
There was a problem hiding this comment.
yeah, maybe it is actually better, that we blow it up, when the user requests us to provide the bucketfs tools, but didn't configure them correctly.
ahsimb
requested a deployment
to
manual-approval
GitHub Actions
Waiting
| kwargs = {k.split("|")[0]: v for k, v in kwargs.items()} | ||
| if saas_env_complete(env): | ||
| kwargs["backend"] = "saas" | ||
| # This code should be moved to the `bucketfs-python.build_path`. |
There was a problem hiding this comment.
What exactly needs to go to bucketfs-python
There was a problem hiding this comment.
At the moment build_path requires the database_id for saas. infer_path makes it more user friendly, allowing to use a database_name instead. Unfortunately infer_path is at odds with the pytest-plugins, so I prefer to use build_path here (I already have a function to infer the backend). But I want the build_path to have the same feature. It would be best to mover this feature from infer_path to build_path.
|
closes #123
This PR doesn't include the update of the User Guide. Will be done in a following PR.