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

Cover Offset() in aggregations with e2e tests #42455

Merged
merged 33 commits into from May 13, 2024
Merged

Conversation

kamilmielnik
Copy link
Contributor

@kamilmielnik kamilmielnik commented May 9, 2024

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:

@kamilmielnik kamilmielnik self-assigned this May 9, 2024
@kamilmielnik kamilmielnik added the no-backport Do not backport this PR to any branch label May 9, 2024
@kamilmielnik kamilmielnik changed the base branch from master to 42377-nested-offset May 10, 2024 09:45
Base automatically changed from 42377-nested-offset to master May 10, 2024 13:39
@@ -0,0 +1,29 @@
export function uuid() {
Copy link
Contributor Author

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).

@kamilmielnik kamilmielnik marked this pull request as ready for review May 13, 2024 09:38
@kamilmielnik kamilmielnik requested review from a team and camsaul May 13, 2024 09:39
@metabase-bot metabase-bot bot added the visual Run Percy visual testing label May 13, 2024
Copy link

Codenotify: Notifying subscribers in CODENOTIFY files for diff f847425...6631cdd.

Notify File(s)
@alxnddr frontend/src/metabase/visualizations/visualizations/SmartScalar/SettingsComponents/SmartScalarSettingsWidgets.tsx
frontend/src/metabase/visualizations/visualizations/SmartScalar/utils.ts

Copy link

replay-io bot commented May 13, 2024

Status Complete ↗︎
Commit 6631cdd
Results
⚠️ 5 Flaky
2502 Passed

Copy link
Contributor

@uladzimirdev uladzimirdev left a 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 */
Copy link
Contributor

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?

Copy link
Contributor Author

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

@kamilmielnik kamilmielnik enabled auto-merge (squash) May 13, 2024 10:46
@kamilmielnik kamilmielnik merged commit f596f0b into master May 13, 2024
191 of 209 checks passed
@kamilmielnik kamilmielnik deleted the 42452-offset-e2e-tests branch May 13, 2024 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/QueryingComponents visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover Offset() in aggregations with e2e tests
2 participants