-
Notifications
You must be signed in to change notification settings - Fork 71
Unit tests for TranspilerRepository
, and additional integration tests
#2042
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
base: main
Are you sure you want to change the base?
Conversation
In addition, we only load configurations in a lazy manner now.
✅ 36/36 passed, 2 flaky, 1m16s total Flaky tests:
Running from acceptance #2395 |
…e installed transpilers (#2051) ## Changes ### What does this PR do? This PR implements a `describe-transpile` subcommand that describes the currently installed transpilers and associated dialects and configuration. This is intended for diagnostics and use by the UI. When run normally, it provides output like this: ``` % databricks labs lakebridge describe-transpile Transpiler Installed Version Plugin Configuration ========== ================= ==================== Morpheus 0.6.6 /Users/me/.databricks/labs/remorph-transpilers/databricks-morph-plugin/lib/config.yml Bladebridge 0.1.15 /Users/me/.databricks/labs/remorph-transpilers/bladebridge/lib/config.yml Supported Source Dialects ========================= - datastage - informatica (desktop edition) - informatica cloud - mssql - netezza - oracle - snowflake - synapse - teradata - tsql ``` When the `--output=json` option is provided to the Databricks CLI, more information is available: ```json % databricks labs lakebridge describe-transpile --output=json { "available-dialects": [ "datastage", "informatica (desktop edition)", "informatica cloud", "mssql", "netezza", "oracle", "snowflake", "synapse", "teradata", "tsql" ], "installed-transpilers": [ { "config-path":"/Users/andrew.snare/.databricks/labs/remorph-transpilers/databricks-morph-plugin/lib/config.yml", "name":"Morpheus", "supported-dialects": { "snowflake": { "options": [] }, "tsql": { "options": [] } }, "versions": { "installed":"0.6.6", "latest":null } }, { "config-path":"/Users/andrew.snare/.databricks/labs/remorph-transpilers/bladebridge/lib/config.yml", "name":"Bladebridge", "supported-dialects": { "datastage": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" }, { "choices": [ "SPARKSQL", "PYSPARK" ], "flag":"target-tech", "method":"CHOICE", "prompt":"Specify which technology should be generated" } ] }, "informatica (desktop edition)": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" }, { "choices": [ "SPARKSQL", "PYSPARK" ], "flag":"target-tech", "method":"CHOICE", "prompt":"Specify which technology should be generated" } ] }, "informatica cloud": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "mssql": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "netezza": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "oracle": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "synapse": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] }, "teradata": { "options": [ { "default":"\u003cnone\u003e", "flag":"overrides-file", "method":"QUESTION", "prompt":"Specify the config file to override the default[Bladebridge] config - press \u003center\u003e for none" } ] } }, "versions": { "installed":"0.1.15", "latest":null } } ] } ``` ### Relevant implementation details The formatting of the details is handled by a new `TranspilerDescription` class, so that the output format can be properly controlled. (For compatibility this will need to be controlled tightly, even if the internal details change.) Currently there is no lookup to figure out the latest version of an installed transpiler, that's out of scope for this PR although it has a position in the JSON output. ### Linked issues Additional tests in the areas modified by this code are implemented in: - #2041 - #2042 ### Functionality - added new CLI command: `databricks labs lakebridge describe-transpile` ### Tests - manually tested - added unit tests - added integration tests
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.
LGTM
This is a flaky test on |
The choices field for dialect-specific options now defaults to None instead of an empty list.
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.
Thank you for the thorough testing. a good example PR on how to write tests
I added a few questions inline that will guide the review forward. Regarding my remark about the interface, I dont expect to solve this in this PR but we should create a follow up to address this. because this point blocked my PR about the transpiler version telemetry so would be helpful to have a plan forward
""" | ||
all_configs = self._all_transpiler_configs() | ||
return {config.name: config for _, config in all_configs} | ||
return {config.name: config for _, config in self._all_transpiler_configs()} |
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.
I feel we really need top standardize this. as discussed before, transpiler path is the ultimate identifier of the transpiler but all the methods that deal with transpiler pathes are private.
a few methods return transpiler names instead of path then the public interface is mostly using transpiler names not pathes.
in some places it is called product name and in other name or transpiler name. we should keep one going forward. please check my other comment
|
||
transpiler_names = transpiler_repository.all_transpiler_names() | ||
|
||
assert transpiler_names == {"A Transpiler", "B Transpiler"} |
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.
should we add a test for all_transpiler_configs()
for this scenario
|
||
|
||
def test_get_installed_transpiler_version() -> None: | ||
"""Verify that the installed version of a transpiler can be queried.""" |
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.
in some places we treat product names as transpiler names and in other places not
assert transpilers == {'Bladebridge'} | ||
|
||
|
||
@pytest.mark.parametrize(("product_name", "version"), (("morpheus", "0.4.0"), ("bladebridge", "0.1.9"))) |
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.
is this a moving target?
assert config_path.is_file() | ||
|
||
|
||
@pytest.mark.parametrize( |
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.
same
Changes
This PR implements unit tests for the
TranspilerRepository
class, as well as extending the coverage of the integration tests. This is aimed mainly at capturing existing behaviour and ensuring bugs are not introduced.Tests