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
Cover Offset()
in aggregations with e2e tests
#42455
Conversation
- it wasn't possible to import the utils file in e2e tests without it
@@ -0,0 +1,29 @@ | |||
export function uuid() { |
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 moved uuid
to a separate file because I wanted to import it in e2e tests, but importing metabase/lib/utils.ts
file in e2e tests caused a webpack error due to some non-importable asset being referenced in metabase/plugins
.
I also renamed isUUID
to isUuid
and made it into a type guard (: uuid is string
).
Codenotify: Notifying subscribers in CODENOTIFY files for diff f847425...6631cdd.
|
|
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.
code looks good 👍
createQuestion({ query }, { visitQuestion: true }); | ||
|
||
verifyNoQuestionError(); | ||
/* TODO: assert actual values */ |
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.
are you going to merge it like this? or is it a leftover?
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.
Merge like this for now.
This test is a repro for not-yet-fixed bug and is skipped for now.
I added info about updating the assertion in the relevant issue: #42554
Closes #42452
Description
e2e tests for the
Offset()
function - only in custom aggregation expressions for now.Custom columns and custom filter expressions are not going into v50 (see Slack).
Issues found because of this PR:
Offset()
#42554