-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: metrics e2e #219
base: main
Are you sure you want to change the base?
chore: metrics e2e #219
Conversation
…e/miguel/metric-e2e
test('Filtering by Label', async ({ selectMetricView }) => { | ||
await selectMetricView.setAdHocFilter('label with 📈', 'metrics'); | ||
|
||
await expect(selectMetricView.getByText('label with 📈 = metrics')).toBeVisible(); |
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.
Could we be more precise and create a method like selectMetricView.assertAdHocFilters(...)
that will target the part of the DOM where the filters are?
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.
they are just content assertions like on the other tests on this file tho
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.
Indeed, IMO the advantage of creating a method are:
- Semantic & consistency: we have
setAdHocFilter
, it feels logical to haveassertAdHocFilters
- Precision: what if "label with 📈 = metrics" or appears elsewhere in the UI?
What advantage(s) do you see not to do so?
…e/miguel/metric-e2e
@@ -17,6 +17,7 @@ | |||
"e2e:fast": "playwright test --retries=2 --workers=3 -x --trace on", | |||
"e2e:debug": "playwright test --debug -x --trace on /test", | |||
"e2e:watch": "playwright test --ui", | |||
"e2e:codegen": "playwright codegen localhost:3000/drilldown", |
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.
"e2e:codegen": "playwright codegen localhost:3000/drilldown", | |
"e2e:codegen": "playwright codegen", |
We can always pass parameters to the npm script so that it works for everyone in the team. For instance, to test WingMan I'm using:
npm run e2ecodegen -- http://localhost:3001/a/grafana-metricsdrilldown-app
await expect(selectMetricView.getByText('Bookmark created')).toBeVisible(); | ||
await selectMetricView.seeAllBookmarksFromAlert(); | ||
// One for recent exploration and one for bookmark | ||
await expect(selectMetricView.getByRole('button', { name: panelTitle })).toHaveCount(2); |
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.
What if there's a bug and 2 bookmarks are created instead of one? This test would pass. Why not be more precise and target the bookmarks container?
As we use fixtures, we could have a "hasBookmark(panelTitle)" method or similar, wdyt?
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 job! I've left a couple of comments that should be easy to address.
Based of: https://docs.google.com/document/d/1kQUkpgKv7ocDGbcrwhPWO-1fgI0bKygeubqE1P8-siA/edit?tab=t.0#heading=h.n352bd4bm30c
Adds fixtures and tests for:
🟩Metric
Selecting breakdown by label
🟩Breakdown
Selecting filters
🟩Actions
Open in explore
Copy url
Bookmark
Select new metric