E2E: Improve support to interact with panel edit options #1272

mckn · 2024-10-29T10:32:36Z

What this PR does / why we need it:
This PR introduce an abstraction to interact with the panel edit options. This is useful when developing panel plugins and you want to verify that different combinations of settings works as expected. The goal is to make it easier and more robust to write tests that interacts with the panel editor. This will also give us a layer where we can cater for differences in the panel editor between Grafana versions.

One example of this is the recent change of the switch component that changed role from checkbox to switch. Developers writing tests interacting with the switch directly would need to cater for this in the test which will make it harder and more complex to test your plugin against multiple versions of Grafana.

Note: A more robust way to test this is to provision one panel per setting. So you never interact with the panel editor while running tests. You only test that the setting has the expected behaviour on your panel. This might not be possible in all scenarios e.g. when adding a custom panel editor or having cascading settings.

This is how the configuration of the panel edit options map into the test APIs.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This is a suggestion and I would like to get more eyes on this to get better naming, structure and ways to interact with the panel editor in your test.

On thing that isn't that great is that while writing the tests you use the "labels" visible in the UI to select options to interact with. This is great for the options that the plugin adds because the plugin controls the labels that they add. But for the standard options or options that is added by Grafana we can still run into the problem that a plugin developer writes a test and interacts with the Standard options label. If we, for some reason, change that in a Grafana version the tests will stop working and the tests will not be able to run across multiple versions of Grafana.

github-actions · 2024-10-29T10:32:56Z

Hello! 👋 This repository uses Auto for releasing packages using PR labels.

✨ This PR can be merged but will not trigger a new release. To trigger a new release add the release label before merging.
NOTE: When merging a PR with the release label please avoid merging another PR. For further information see here.

sunker

Nice! I think I prefer option 1 as option 2 would make us slightly more vulnerable to changes in the core Playwright api.

Question: Would it be better to replace input and switch with getByRole(role: 'input' | 'switch' | ..., { name: string }: Options)? This would make our abstraction look closer to the playwright api?

const group = panelEditPage.getOptionsGroup('Clock panel')
await group.getByRole('switch', { name: 'enableClock' }).uncheck()

mckn · 2024-10-29T16:41:33Z

Nice! I think I prefer option 1 as option 2 would make us slightly more vulnerable to changes in the core Playwright api.

Question: Would it be better to replace input and switch with getByRole(role: 'input' | 'switch' | ..., { name: string }: Options)? This would make our abstraction look closer to the playwright api?
const group = panelEditPage.getOptionsGroup('Clock panel')
await group.getByRole('switch', { name: 'enableClock' }).uncheck()

Yeah, but on the other hand 'input' is not a role. In the scenario where with the switch-bug you would still need to know if you are trying to fetch a combobox or a switch role.

mckn · 2024-10-29T18:13:37Z

@sunker I added some more code and tests. Lets have a sync about this before I continue further but it feels kind of OK to work with when writing tests.

sunker · 2024-10-30T07:16:37Z

Nice! Great tests!

I had a quick look at the helper methods in the panel options API and this is what you can do:

addNestedOptions (can skip this for now)
addNumberInput (textbox)
addSliderInput (not sure, textbox I guess?)
addTextInput (textbox or textarea)
addSelect (combobox)
addMultiSelect (combobox)
addRadio (radiogroup)
addBooleanSwitch (switch in recent versions) 
addColorPicker (don't know - can probably skip for now)
addTimeZonePicker (combobox)
addUnitPicker (comobobox)
addFieldNamePicker (combobox)
addDashboardPicker (combobox)

So basically we need to support textbox, textarea, combobox, radiogroup and switch. So I guess we'll have to choose between:

group.getByRole(role: textbox | textarea | combobox | radiogroup | switch, options: { ... })
// or implementing each specifically like in this PR. e.g
group.getRadioButtonGroup(label: string)

I don't have strong opinions here. The tricky bit will be to get them to work in >= 8.5.0.

mckn · 2024-10-30T08:58:21Z

Nice! Great tests!

I had a quick look at the helper methods in the panel options API and this is what you can do:
addNestedOptions (can skip this for now)
addNumberInput (textbox)
addSliderInput (not sure, textbox I guess?)
addTextInput (textbox or textarea)
addSelect (combobox)
addMultiSelect (combobox)
addRadio (radiogroup)
addBooleanSwitch (switch in recent versions) 
addColorPicker (don't know - can probably skip for now)
addTimeZonePicker (combobox)
addUnitPicker (comobobox)
addFieldNamePicker (combobox)
addDashboardPicker (combobox) 
So basically we need to support textbox, textarea, combobox, radiogroup and switch. So I guess we'll have to choose between:
group.getByRole(role: textbox | textarea | combobox | radiogroup | switch, options: { ... })
// or implementing each specifically like in this PR. e.g
group.getRadioButtonGroup(label: string)
I don't have strong opinions here. The tricky bit will be to get them to work in >= 8.5.0.

I'm happy with both as well. The only thing I feel is a bit negative with the getByRole is that it diverge from the standard getByRole since we are introducing "new" roles.

But I'm happy to proceed with that approach as well. Maybe ask for some more input here?

sunker · 2024-10-30T09:07:11Z

The only thing I feel is a bit negative with the getByRole is that it diverge from the standard getByRole since we are introducing "new" roles.

You mean because they only make up a subset of all roles that exists?

Maybe ask for some more input here?

Sounds good! Maybe Jack? He's affected by the recent change in his traffic light panel. :)

mckn · 2024-10-30T09:14:02Z

You mean because they only make up a subset of all roles that exists?

Yes, and we would need to have custom roles that match to our components. E.g. in the scenario with the recent change the switch got changed from a 'checkbox' to a 'switch' and in those cases we would not want the developer to need to update their tests. In case they have getByRole('checkbox', ...);.

I understood your suggestion as we don't use the default roles but use our own that maps to our components rather than the html roles. Otherwise we don't resolve issues as the one with the switch.

packages/plugin-e2e/src/models/components/Select.ts

packages/plugin-e2e/src/models/components/PanelEditOptionsGroup.ts

…t options.

…oup and switch.

sunker · 2024-12-04T06:53:21Z

packages/plugin-e2e/src/matchers/toHaveSelected.ts

+    actual = await select
+      .locator('div[class*="-grafana-select-value-container"]')
+      .locator('div[class*="-singleValue"]')
+      .innerText(options);


Isn't there an e2e selector for this? If not, we should add one to Grafana.

One problem with this and the other new matchers is that that they don't have access to the test context. There's no way for them to check grafanaVersion and use the resolved selectors.

But IDK - the Selector class has access to the context, so maybe it can be encapsulated there instead?

hmm, that is a valid concern. But I think we can fix that in the index.ts file.

The ctx is available with that the selectors as well. So I will see if I can change these to selectors from the grafana-selectors package istead.

How? This is fine in older versions of Grafana, but we need a stable e2e-selector in Grafana to be future proof. Looks like there's a data-testid set here. Can we use that instead of the classnames?

hmm, I don't think we pass the data-testid when rendering the select component. Or I can't find it in the dom.

Okay, can we add a new one then?

packages/plugin-e2e/src/models/components/ColorPicker.ts

packages/plugin-e2e/src/models/components/ComponentBase.ts

sunker · 2024-12-04T07:01:25Z

packages/plugin-e2e/src/models/components/Select.ts

+
+export async function openSelect(element: Locator, options?: SelectOptionsType): Promise<Locator> {
+  await element.getByRole('combobox').click(options);
+  return element.page().getByLabel('Select options menu', {


Use selectors.components.Select.menu instead

A slightly different selector got added in 11.4

https://github.com/grafana/grafana/blob/main/packages/grafana-e2e-selectors/src/selectors/components.ts#L881

Okay, nice. Let's use than in +11.4

Let's use the selector here too

packages/plugin-e2e/src/models/components/Select.ts

packages/plugin-e2e/src/models/pages/PanelEditPage.ts

packages/plugin-e2e/src/models/components/PanelEditOptionsGroup.ts

packages/plugin-e2e/src/matchers/toBeSwitched.ts

packages/plugin-e2e/tests/as-admin-user/panel/panelEdit.spec.ts

packages/plugin-e2e/src/models/components/PanelEditOptionsGroup.ts

sunker

This is starting to look really good!

sunker · 2024-12-11T07:49:36Z

packages/plugin-e2e/src/matchers/toHaveSelected.ts

+    actual = await select
+      .locator('div[class*="-grafana-select-value-container"]')
+      .locator('div[class*="-singleValue"]')
+      .innerText(options);


How? This is fine in older versions of Grafana, but we need a stable e2e-selector in Grafana to be future proof. Looks like there's a data-testid set here. Can we use that instead of the classnames?

packages/plugin-e2e/src/models/components/ColorPicker.ts

sunker · 2024-12-11T07:55:23Z

packages/plugin-e2e/src/models/components/ColorPicker.ts

+    }
+
+    await this.ctx.page
+      .locator('#grafana-portal-container')


Here as well. This is fine in older versions of Grafana, but now can we add a selector to this element?

packages/plugin-e2e/src/models/components/ComponentBase.ts

sunker · 2024-12-11T08:01:26Z

packages/plugin-e2e/src/models/components/Select.ts

+
+export async function openSelect(element: Locator, options?: SelectOptionsType): Promise<Locator> {
+  await element.getByRole('combobox').click(options);
+  return element.page().getByLabel('Select options menu', {


Okay, nice. Let's use than in +11.4

sunker

Great stuff! From my standpoint we're ready to ship this, but I think we should try to add proper selector for a few of the elements in Grafana first.

sunker · 2024-12-12T07:37:15Z

.github/workflows/playwright.yml

@@ -25,7 +25,7 @@ jobs:
        with:
          version-resolver-type: plugin-grafana-dependency
          grafana-dependency: '>=8.5.0'
-          # limit: 0 # Uncomment to test all versions since 8.5.0. Useful when testing compatibility for new APIs.
+          limit: 0


now that all tests pass, we can revert the change in this file :)

sunker · 2024-12-12T07:39:45Z

packages/plugin-e2e/src/matchers/toHaveSelected.ts

+    actual = await select
+      .locator('div[class*="-grafana-select-value-container"]')
+      .locator('div[class*="-singleValue"]')
+      .innerText(options);


Okay, can we add a new one then?

sunker · 2024-12-12T07:40:54Z

packages/plugin-e2e/src/models/components/ColorPicker.ts

+    if (gte(this.ctx.grafanaVersion, '8.7.0')) {
+      return this.ctx.page.locator('#grafana-portal-container');
+    }
+    return this.ctx.page.locator('body > div').last();


if you could add a new testid in Grafana and use it here that would be great!

sunker · 2024-12-12T07:42:26Z

packages/plugin-e2e/src/models/components/Select.ts

+
+export async function openSelect(element: Locator, options?: SelectOptionsType): Promise<Locator> {
+  await element.getByRole('combobox').click(options);
+  return element.page().getByLabel('Select options menu', {


Let's use the selector here too

sunker · 2024-12-12T07:43:54Z

packages/plugin-e2e/src/models/components/Switch.ts

+    }
+
+    const id = await this.element.getAttribute('id', options);
+    return this.group.locator(`label[for='${id}']`);


Nit: Maybe reverse the order here so that backwards compatibility becomes the "exception" rather than the default case. That way it will be easier to remove legacy code in the future.

sunker · 2024-12-12T07:44:11Z

packages/plugin-e2e/src/models/components/UnitPicker.ts

+
+  private async getOption(selector: string, options?: SelectOptionsType): Promise<Locator> {
+    const steps = selector.split('>').map((step) => step.trim());
+    const container = this.ctx.page.locator('div[class="rc-cascader-menus"]');


Can we introduce a new data test id here too?

sunker · 2024-12-12T07:45:58Z

packages/plugin-e2e/tests/as-admin-user/panel/panelEdit.spec.ts

+
+function getPanelContent(grafanaVersion: string, page: Page): Locator {
+  if (gte(grafanaVersion, '9.5.0')) {
+    return page.locator('div[class*="-panel-content"]');


Can use selectors.components.Panels.Panel.content?

sunker reviewed Oct 29, 2024

View reviewed changes

mckn self-assigned this Oct 30, 2024

mckn added plugin-e2e related to the plugin-e2e package minor Increment the minor version when merged labels Oct 30, 2024

mckn commented Oct 30, 2024

View reviewed changes

packages/plugin-e2e/src/models/components/Select.ts Outdated Show resolved Hide resolved

sunker reviewed Oct 30, 2024

View reviewed changes

packages/plugin-e2e/src/models/components/PanelEditOptionsGroup.ts Outdated Show resolved Hide resolved

mckn requested a review from jackw October 31, 2024 15:47

mckn requested a review from sunker December 3, 2024 20:12

mckn added 11 commits December 3, 2024 21:18

Added two suggestions on how to implement the selectors for panel edi…

e0abc09

…t options.

Added some tests and implemented functions for input, radio button gr…

f82174a

…oup and switch.

Added support for select.

03ab082

added some more tests and apis.

95e8075

added support for unit picker.

03dbc35

verified time zone.

6a75306

wip - proxy hack.

4ea35b0

wip

ef91a82

Changed the APIs to be playwright-like.

b5a1776

added unit picker as well.

6f3f4ac

removed unused ctx prop.

ef666ca

mckn force-pushed the mckn/panel-edit-options-group branch from d073a1d to ef666ca Compare December 3, 2024 20:18

removed unused import.

3441a14

sunker reviewed Dec 4, 2024

View reviewed changes

mckn commented Dec 4, 2024

View reviewed changes

packages/plugin-e2e/src/models/components/PanelEditOptionsGroup.ts Outdated Show resolved Hide resolved

sunker and others added 8 commits December 4, 2024 11:03

use all supported versions in matrix

c73e46f

Made it work in 11.3.x

dd03be6

fixed selectors for v11.2.*

09e628a

made it work for 11.1.*

fffa740

Fixed for 10.3.*

82629ba

Removed comment.

858b5fe

supporting 8.5.x

52fdb3d

Merge branch 'main' into mckn/panel-edit-options-group

b8d32a2

sunker mentioned this pull request Dec 10, 2024

Skip failing test grafana/grafana-plugin-examples#441

Merged

mckn added 3 commits December 10, 2024 10:34

removed the need of having switch async.

9a76a3a

removed private function.

64160a0

renamed switched to checked.

7bbe0bd

mckn marked this pull request as ready for review December 10, 2024 09:53

mckn requested a review from a team as a code owner December 10, 2024 09:53

mckn added 3 commits December 10, 2024 10:58

added ctx.

c36f555

minor refactoring to have access to ctx via component base.

44c31f3

removed unneccessary check.

bd9eac6

sunker mentioned this pull request Dec 10, 2024

Plugin E2E: Fix some APIs that were broken in older versions of Grafana #1391

Merged

mckn added 3 commits December 11, 2024 08:26

Merge branch 'main' into mckn/panel-edit-options-group

3570835

using selector instead.

1138214

Merge branch 'main' into mckn/panel-edit-options-group

94798d1

sunker reviewed Dec 11, 2024

View reviewed changes

sunker mentioned this pull request Dec 11, 2024

Update grafana dependencies #1383

Open

mckn added 5 commits December 11, 2024 16:29

allow any rgb or hex color to be selected.

8bb16e9

Fixed so the switch works again.

a410338

chaninging to checkbox for switch in 11.4.0

760ad9e

fixed slider in 9.1.8

6d42378

Merge branch 'main' into mckn/panel-edit-options-group

57e103c

mckn requested a review from sunker December 11, 2024 16:59

sunker reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2E: Improve support to interact with panel edit options #1272

E2E: Improve support to interact with panel edit options #1272

mckn commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

sunker left a comment

mckn commented Oct 29, 2024

mckn commented Oct 29, 2024

sunker commented Oct 30, 2024 •

edited

Loading

mckn commented Oct 30, 2024

sunker commented Oct 30, 2024

mckn commented Oct 30, 2024

sunker Dec 4, 2024

sunker Dec 4, 2024

mckn Dec 4, 2024

mckn Dec 11, 2024

sunker Dec 11, 2024

mckn Dec 11, 2024

sunker Dec 12, 2024

sunker Dec 4, 2024

mckn Dec 11, 2024

sunker Dec 11, 2024

sunker Dec 12, 2024

sunker left a comment

sunker Dec 11, 2024

sunker Dec 11, 2024

sunker Dec 11, 2024

sunker left a comment

sunker Dec 12, 2024 •

edited

Loading

sunker Dec 12, 2024

sunker Dec 12, 2024

sunker Dec 12, 2024

sunker Dec 12, 2024

sunker Dec 12, 2024

sunker Dec 12, 2024

E2E: Improve support to interact with panel edit options #1272

Are you sure you want to change the base?

E2E: Improve support to interact with panel edit options #1272

Conversation

mckn commented Oct 29, 2024 • edited Loading

github-actions bot commented Oct 29, 2024 • edited Loading

sunker left a comment

Choose a reason for hiding this comment

mckn commented Oct 29, 2024

mckn commented Oct 29, 2024

sunker commented Oct 30, 2024 • edited Loading

mckn commented Oct 30, 2024

sunker commented Oct 30, 2024

mckn commented Oct 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunker left a comment

Choose a reason for hiding this comment

sunker Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckn commented Oct 29, 2024 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

sunker commented Oct 30, 2024 •

edited

Loading

sunker Dec 12, 2024 •

edited

Loading