Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

lpoulain
Copy link
Collaborator

@lpoulain lpoulain commented May 2, 2024

Here are the changes that needed to be made to move to Metabase 1.49.3:

  • The sample-dataset dataset was renamed test-data
  • The 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
  • Some further Metabase tests which are not expected to work on Presto/Trino were introduced, so had to branch two new Metabase tests files to remove the offending tests. This increases the burden when upgrading the version of Metabase, but unfortunately I haven't found any other way

@lpoulain lpoulain linked an issue May 2, 2024 that may be closed by this pull request
Copy link
Member

@andrewdibiasio6 andrewdibiasio6 left a 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).
Copy link
Member

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?

Copy link
Collaborator Author

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
Copy link
Member

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?

Copy link
Collaborator Author

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

@lpoulain
Copy link
Collaborator Author

Cancelling as waiting for Metabase 1.50 which have better tests integration

@lpoulain lpoulain closed this May 22, 2024
@lpoulain lpoulain deleted the 128-upgrade-to-metabase-1-49-3 branch July 2, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to Metabase 1.50
2 participants