-
Notifications
You must be signed in to change notification settings - Fork 10
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
Upgrade to Metabase 1.49.3 and fix the tests accordingly #129
Conversation
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.
Some comments about us working with them an disabling tests on their master branch if needed.
@@ -64,9 +64,9 @@ Head to actions and run the `Release` workflow entering the same the same semant | |||
### Update Metabase Version | |||
If needed, `make checkout_latest_metabase_tag` will update Metabase to its latest tagged release. | |||
|
|||
*CAUTION*: the Metabase test file `metabase/test/metabase/driver_test.clj` is overridden by a modified version on the root directory (see the `Makefile`). This is because two tests (`can-connect-with-destroy-db-test` and `check-can-connect-before-sync-test`) do not work with the Starburst driver as they're testing what happens when a database is deleted (which cannot happen with Starburst). So instead of adding some useless stuff to `can-connect?` for the sole purpose of satisfying tests, it was found preferable to just remove those two tests. | |||
*CAUTION*: several Metabase test files (`driver_test.clj`, `dataset_definition_test.clj` and `connection_test.clj`) are overridden by a modified version on the root directory (see the `Makefile`). This is because several tests (`can-connect-with-destroy-db-test`, `check-can-connect-before-sync-test`, `dataset-with-custom-pk-test`, `dataset-with-custom-composite-pk-test` and `test-ssh-tunnel-connection`) are not expected to work for any Presto/Trino driver. They are explicitly disabled for the Presto and Athena drivers but not the Starburst driver, leading to failures (most of these test foreign keys, something Trino doesn't support). |
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.
ey are explicitly disabled for the Presto and Athena drivers but not the Starburst driver,
Can we sync with metabase to disable them? If we need to, we should see if they disable all foreign key tests for SB?
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've reached out to Metabase but so far no luck. They cannot add an exception to third-party drivers in their own code. I've asked for a way to have a class of drivers (e.g. Presto / Athena) we could subscribe to but there is no immediate plan for that.
@@ -0,0 +1,350 @@ | |||
(ns metabase.driver.sql-jdbc.connection-test |
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.
So what changed in this file?
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 made the test-ssh-tunnel-connection
test into a no-op
Cancelling as waiting for Metabase 1.50 which have better tests integration |
Here are the changes that needed to be made to move to Metabase 1.49.3:
sample-dataset
dataset was renamedtest-data
describe-table-fks
method is now deprecated. We don't use it but still define it, so I updated the method to mimic what the Presto driver is doing