-
Notifications
You must be signed in to change notification settings - Fork 2
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 Metabase to v0.51 #60
Conversation
375c85b
to
dee5272
Compare
dee5272
to
6ac710b
Compare
28f5a79
to
9b68303
Compare
Currently excluding a couple of tests some of which are not applicable to Materialize:
We can follow up on those few tests during the upgrade to v52 and also open tracking issues for them in the meantime. |
(is (= 1000 | ||
(metadata-queries/field-count (t2/select-one Field :id (mt/id :checkins :venue_id))))))) | ||
|
||
-(deftest ^:parallel table-rows-sample-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.
Shoud this test be skipped for drivers with :expressions
disabled?
[2 "bar" #t "2020-04-21T16:43"] | ||
[3 "baz" #t "2021-04-21T16:43"]])) | ||
|
||
-(deftest ^:parallel yyyymmddhhmmss-binary-dates |
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.
Failing with:
org.postgresql.util.PSQLException: ERROR: function to_timestamp(text, unknown) does not exist
Hint: No function matches the given name and argument types. You might need to add explicit type casts.
(cond | ||
;; There's a bug here where we are reading in the UTC time as pacific, so we're 7 hours off | ||
;; (This is fixed for Oracle now) | ||
- (and (qp.test-util/tz-shifted-driver-bug? driver) (not= driver :oracle)) |
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 as Oracle, it will be nice if this test was behind a feature flag?
scripts/exclude_tests.diff
Outdated
;;; | ||
;;; TODO -- not sure what exactly this means. Maybe it was talking about marking FKs automatically during sync? Since we | ||
;;; now do that manually for DBs like MongoDB maybe we can enable these tests for Mongo. | ||
-(defmethod driver/database-supports? [:mongo ::foreign-keys-as-required-by-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.
Same as Mongo, it will be nice if this test was behind a feature flag?
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.
You should be able to place this snippet inside metabase.test.data.materialize
(defmethod driver/database-supports? [:materialize ::foreign-keys-as-required-by-tests]
[_driver _feature _database]
false)
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.
Nice, thanks! Just updated that.
- ["user" | ||
- [{:field-name "custom_id" :base-type :type/Integer :pk? true}] | ||
- [[1]]] | ||
- ["group" | ||
- [{:field-name "user_custom_id" :base-type :type/Integer :fk "user"}] | ||
- [[1]]]) |
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.
This is the (deftest dataset-with-custom-pk-test
test, judging by its name it sounds like should be excluded by default for drivers with :metadata/key-constraints
?
Fixes #57