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

Plugin E2E: Enable assertion on panel data #688

Merged
merged 5 commits into from
Feb 10, 2024
Merged

Conversation

sunker
Copy link
Contributor

@sunker sunker commented Jan 23, 2024

What this PR does / why we need it:

Plugin E2E currently provides two ways to assert that a data source query works as expected:

  • using the toBeOK matcher to check that the data query response was ok
    e.g await expect(explorePage.runQuery()).toBeOK();
  • using the hasPanelError matcher to checks weather the panel has an error icon

As @yesoreyeram pointed out, we need a way to check what data is being displayed in the panel too. That way, one can assert that the entire data flow of a plugin (data source plugin in particular) works as expected.

Asserting on the data that is being displayed in a panel is tricky as the markup is a moving target. For that reason, it's important that we somehow incapsulate panel data extraction within plugin-e2e so we have a chance to fix the parsing whenever the panel markup changes. I was initially considering writing a plugin-e2e custom matcher to cater for this. However, that may lead to a scenario where we need to provide a whole bunch of different matchers to allow users to find assert on single value, multiple values, substring searches etc. It would be better if we could find a way to use the built-in Playwright matchers.

In this PR, the Panel model exposes two methods - getFieldNames and getData. getFieldNames returns a locator that resolves to element(s) that point to data frame field name, and getData returns a locator that resolves to element(s) that point to data frame values. Since they both return a Playwright locator, it can be used together with any of built-in playwright matchers. Currently, this only works for the table panel. That is likely the only panel you'd want to use in an E2E test anyway, but if another panel is being used one can always toggle View table (in panel edit), and then assertion should work.

e.g

test('table panel data assertions', async ({ page, selectors, grafanaVersion, request, readProvision }) => {
    const dashboard = await readProvision<Dashboard>({ filePath: 'dashboards/redshift.json' });
    const panelEditPage = new PanelEditPage({ page, selectors, grafanaVersion, request }, { dashboard, id: '3' });
    await panelEditPage.goto();
    await panelEditPage.setVisualization('Table');
    await expect(panelEditPage.panel.getData()).toContainText(['staging']);
    await expect(panelEditPage.panel.getFieldNames()).toContainText(['time', 'temperature']);
});

You'll find more examples of how data assertions can be done in provisioned dashboard, explore page etc in the dataAssertion.spec.ts file.

📦 Published PR as canary version: Canary Versions

✨ Test out this PR locally via:

npm install @grafana/plugin-e2e@0.13.0-canary.688.5df6275.0
# or 
yarn add @grafana/plugin-e2e@0.13.0-canary.688.5df6275.0

Copy link

github-actions bot commented Jan 23, 2024

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

✨ This PR can be merged and will trigger a new minor release.
NOTE: When merging a PR with the release label please avoid merging another PR. For further information see here.

}

getLabels(): Locator {
return this.panel.locator('[role="columnheader"]');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course we'd need to add proper e2e selectors to all visualizations in Grafana an use them instead of week magic string like this (the same goes for all following extractors).

@sunker sunker requested a review from academo January 24, 2024 07:47
@sunker sunker marked this pull request as ready for review January 24, 2024 07:47
@sunker sunker requested a review from a team as a code owner January 24, 2024 07:47
@sunker sunker mentioned this pull request Jan 24, 2024
6 tasks
@sunker sunker changed the title [POC] Plugin E2E: Enable assertion on panel data Plugin E2E: Enable assertion on panel data Jan 24, 2024
Copy link

@yesoreyeram yesoreyeram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like a good start. We also need to consider multiple cases and make it more generic. In general, instead of implementing individual methods for panels such as getLabels/getValues, I would like to see getData kind of abstraction which should return array of data frames kind of data used by the viz to render the data. We could use toggle table view and extract data to make it more consistent for the viz.

If I am datasource developer, I will least bother about how different visualisation render my data. Only care about if the visualization getting correct/consistent data from the query.

packages/plugin-e2e/src/models/panels/visualizations.ts Outdated Show resolved Hide resolved
return this.panel.locator('[role="columnheader"]');
}

getValues(): Locator {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we consider the scenario where a data query respond with multi frame?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm that's the wide format right? doesn't the table panel give you the option to change series from a dropdown then? maybe plugin-e2e could provide apis for changing that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the multi frame format where you see the dropdown with frame names. This dashboard may be handy (you can change scenario and play around).

}
};

export class VisualizationDataExtractor {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, when we are are edit panel view it would be easy to extract the data from any visualization by toggle to "table view". Instead of reading data for all the viz in unique way, this will simplify most of the work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah you may be right. I think I mostly wanted to support different visualizations because it would partly enable E2E testing that a provisioned databoard is working as expect.

const dashboard = await readProvision<Dashboard>({ filePath: 'dashboards/redshift.json' });
 const dashboardPage = new DashboardPage({ page, selectors, grafanaVersion, request }, { uid: dashboard.uid });
 await dashboardPage.goto();
 // this is a Gauge panel
 const panel = await dashboardPage.getPanelByTitle('Redshift');
 expect(await panel.getValues()). toHaveValue('test')

But ofc this would work also by switching to table view.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In enterprise datasources, we mostly rely on explore view for simplicity. That workflow directly opens the explore URL with the query passed in the param and then assert the table results. Then optionally you can change the values in the editor to ensure frontend components works as expected. But unfortunately, that workflow doesn't cover cases such as variable interpolation.

@sunker
Copy link
Contributor Author

sunker commented Jan 25, 2024

Thanks for feedback @yesoreyeram!

Yes I like the idea of toggling to table view for all panels. Will make it way less complicated.

I would like to see getData kind of abstraction which should return array of data frames kind of data used by the viz to render the data.

How would we be able to get the data frames passed to a panel from within an E2E test? Checking the Query inspector panel data would be slow and brittle.

I'm not sure you would want to assert on content of the data frames in an E2E test. Even though the format is to be considered stable, it's still moving target so you'd need to change your tests whenever a change is introduced. I think we're better of looking at what's being rendered in the table panel. If the rendering of table data is changed, we would catch that in an E2E test in grafana core and provide a fix in the plugin-e2e parser.

What about if we skip the whole vizualization concept, and instead expose two methods from the panel model - getData and getFieldNames (or getColumnNames). These methods would first toggle to Table view and then return a Playwright locator which will give plugin authors access to the variety of Playwright matchers that already exist (there's a lot of them). Then if there's a need to assert on the shape of the data frames, they can use the json in the response returned by for example panelEditPage.refreshPanel(). It won't cover the scenario where data is being further processed in the frontend, but probably good enough in most cases.

@yesoreyeram
Copy link

I'm not sure you would want to assert on content of the data frames in an E2E test. Even though the format is to be considered stable, it's still moving target so you'd need to change your tests whenever a change is introduced. I think we're better of looking at what's being rendered in the table panel

I completely agree. I might not conveyed correctly. I just want to say that I want the table panel content in a format similar to data frames ( doesn't need to be exact data frames ).

@yesoreyeram
Copy link

If there's a need to assert on the shape of the data frames, they can use the json in the response returned by for example panelEditPage.refreshPanel()

I don't think there is a need to assert the data frame shape. Once we assert the table panel content, that is most sufficient. ( also some datasources does post processing of response in the client side and so in either cases, asserting table panel content is more useful )

@yesoreyeram
Copy link

How would we be able to get the data frames passed to a panel from within an E2E test? Checking the Query inspector panel data would be slow and brittle.

Definitely I didn't meant to read from query inspector :)

@sunker
Copy link
Contributor Author

sunker commented Jan 25, 2024

Awesome @yesoreyeram - I take it we're on the same page and have a path forward. I'll try to update the PR tomorrow. 👍

@sunker sunker added minor Increment the minor version when merged release Create a release when this pr is merged labels Jan 29, 2024
@@ -0,0 +1,14 @@
import { Page } from '@playwright/test';

export const radioButtonSetChecked = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Planning to expose some kind of UI helpers through the base page, but will do that in a different pr.

getData(): Locator {
return this.locator().locator('[role="cell"]');
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I'm working on adding proper e2e selectors for the table panel in grafana core.

Even though people will probably want to use the Table panel in most cases, it won't cover the scenario when you want to assert on panel (that is not table panel) data in a dashboard, as you can't toggle to Table view from there. Not high priority, but we could easily add parsing functionality to some of the most common panel types. E.g

getData(): Locator {
    switch (this.visualizationType) {
      case 'table':
        return this.locator().locator('[role="cell"]');
      case 'logs':
        return this.locator().locator(selectors.components.Panels.Panel.Logs.logRow);
      case 'gauge':
        return this.locator().locator(selectors.components.Panels.Panel.Gauge.value);
      default:
        throw new Error(`No data parser implemented for this visualization type: ${this.visualizationType}`);
    }
  }

@sunker
Copy link
Contributor Author

sunker commented Jan 29, 2024

Okay @yesoreyeram, code and PR description updated. Would be great if you could take another look.

let locator = this.getByTestIdOrAriaLabel(this.ctx.selectors.components.Panels.Panel.title(suffix), {
startsWith: true,
});
private getTablePanelLocator() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

locating table panel and time series panel in explore is a mess. for some reason, selectors seem to change in every version of Grafana. need to look into this at some point and investigate how this can be improved.

Copy link

@yesoreyeram yesoreyeram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Let's hope we will find more usecases and improve this in later iterations.

@sunker sunker merged commit c81c7a7 into main Feb 10, 2024
20 checks passed
@sunker sunker deleted the plugin-e2e/table-assertion branch February 10, 2024 12:30
@grafana-plugins-platform-bot
Copy link

🚀 PR was released in @grafana/plugin-e2e@0.13.0 🚀

@grafana-plugins-platform-bot grafana-plugins-platform-bot bot added the released This issue/pull request has been released. label Feb 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged release Create a release when this pr is merged released This issue/pull request has been released.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants